Add public siftUp/Down methods to PriorityQueue

[Pfiver]

This was just a learning exercise. I actually tried to measure the impact and in fact observed the two-fold performance increase. Full disclosure: The issue came up and I worked on the performance improvement when solving the advent of code 2021 day 15 exercise, just for fun: https://github.com/Pfiver/advent-of-code-2021/blob/main/day15/Solve0.java

I think it would nevertheless be nice to have those methods generally available in the public API.

The semantics, especially the decision to use referential identity instead of equals() equality for finding the updated element in the queue has been guided by performance considerations. They are not very important to me and open for change.

In reference to https://openjdk.java.net/contribute/, the best possible outcome for this PR would IMHO be that a trusted JDK engineer would just pick this code or, more likely, just the idea, up, and push it through the process on their own.

OCA is signed: https://oca.opensource.oracle.com/api/v1/oca-requests/6243/documents/6263/download

To the extent possible under law, I hereby waive all copyright and related or neighboring rights to these Open JDK Priority Queue siftUp/Down implementations, effectively donating it to the public domain This work is published from Switzerland.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6938/head:pull/6938
$ git checkout pull/6938

Update a local copy of the PR:
$ git checkout pull/6938
$ git pull https://git.openjdk.java.net/jdk pull/6938/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6938

View PR using the GUI difftool:
$ git pr show -t 6938

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6938.diff

[bridgekeeper]

Hi @Pfiver, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Pfiver" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

@Pfiver The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[Pfiver]

/signed

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[bridgekeeper]

@Pfiver This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[Pfiver]

/signed

[bridgekeeper]

You are already a known contributor!

[bridgekeeper]

@Pfiver This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[TheShermanTanker]

@Pfiver Would you like me to help you create a corresponding entry in the tracker so your PR is marked as ready for review?

[Pfiver]

Oh. Yes! Sure. I receved instructions to write a mail to core-lib-dev ml but didn't get around doing so yet. Also, tbh it would be more convenient for me to have a discussion about it here on gh than having to go through the list. 03.03.2022 11:11:55 TheShermanTanker ***@***.***>:

…

@Pfiver[https://github.com/Pfiver] Would you like me to help you create a corresponding entry in the tracker so your PR is marked as ready for review? — Reply to this email directly, view it on GitHub[#6938 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAD26SEUIVLBAOYOAJYHZR3U6CF6VANCNFSM5K6CLMMA]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. You are receiving this because you were mentioned. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEgAAABICAYAAABV7bNHAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAArSURBVHic7cEBDQAAAMKg909tDjegAAAAAAAAAAAAAAAAAAAAAAAAAAA+DFFIAAEctgHwAAAAAElFTkSuQmCC###24x24:true###][Verfolgungsbild][https://github.com/notifications/beacon/AAD26SGDMEXFPA3SZVIVXUTU6CF6VA5CNFSM5K6CLMMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOH4HA6XQ.gif]

[TheShermanTanker]

@Pfiver I do find it more convenient to discuss here as well, but unfortunately due to how the workflow is structured I may risk taking up a slot in the tracker for a PR that might not make it in due to miscommunication. I'll notify the mailing list first and point them to this PR, and if no one responds within a day I'll just take the risk and create a new issue for your PR anyway

[TheShermanTanker]

@Pfiver Sorry it's taken me quite a while to get back to this, there seems to be quite a number of issues raised by the core libs team in response to this PR:

Modifying the priorities of elements in the PriorityQueue violates the
invariants of the PriorityQueue established at insertion and maintained
at removal by the Comparator.
To maintain the invariant the element should be removed, its priority modified,
and re-inserted.
An API to manually manipulate the order is inconsistent with the design of
PriorityQueue.

Regards, Roger

I agree with Roger. Let me amplify this.
The general rule for collections that use hashes and comparison methods (such as
HashMap and TreeMap, as well as PriorityQueue) is that one mustn't mutate any
element that resides in such a collection in any way that changes the results of
hashCode, equals, or the comparison method. It's a bad precedent to add APIs that
seem to support such mutation. As Roger said, the supported way of doing this is to
remove, mutate, and then reinsert.
It seems like it might be safe to mutate an element, only temporarily violating the
PQ's invariants until the mutated element is sifted into the correct position.
However, even a "temporary" violation is exceedingly dangerous. If some other
modification is made to the PQ while it's in this state, it could end up permanently
corrupting the PQ.
Managing such a situation would need to be handled exceedingly carefully. As such,
this seems like a highly specialized use case, thus the proposal isn't suitable as a
general-purpose API.

s'marks

[Pfiver]

@TheShermanTanker Cool. Thank you very much for relaying the messages from the list here!

All right, I understand the concerns. My first thought about how to address them would be a functional API. Something like this:

    /**
     * Applies the re-prioritization function {@code mutator}
     * to the item x and relocates it to its proper place in the queue.
     *
     * If the item's priority is raised by the {@code mutator},
     * it will potentially be moved closer to the start.
     *
     * If the item's priority is lowered by the {@code mutator},
     * it will potentially be moved closer to the end.
     *
     * If the {@code mutator} does not change the item's priority
     * it will not be moved.
     *
     * The call has no effect if the item cannot be found in the queue
     * by referential equality.
     *
     * @param x the item to sift up
     * @param mutator a function to be applied to x to change its priority
     */
    public void rePrioritize(E x, Consumer<E> mutator) { ... }

As it is already documented on the class that it is not thread-save, this should get past those concerns then, shouldn't it?

If yes, the next thing to do for me, would be to get creative and implement this in a manner that is equally performant than the original suggestion (e.g. much faster than remove(), mutate(), add()). I am sure that this could be done. If an item's priority was an int or long, it would be trivial. But since items are only comparable, the implementation would be, well, non-trivial.

But before I start wrecking my brain about the implementation, let me take a break here and re-focus. As said, I understand the concerns that have been raised against the initial suggestion (e.g. this PR). And even though an attempt at finding a working solution has not been discouraged so far, at the moment I don't a big enough upside of having these methods available in the API as well, that would justify the investment for me. When making this PR, my motivation was also (1.) that I was curious about the reasons why they are missing and (2.) about the JDK PR process in general. And this curiosity has been satisfied.

1. I was not really aware about the requirements of the API user to maintain the "invariants of the PriorityQueue" while using it. (Btw. I have not found any explicitly documentation of the concept of "[collection] invariants" or "[t]he general rule for collections that use [...] comparison methods" on the class or the add() method. Does anybody think it would make sense to fix that or did I just not see it, maybe elsewhere?)

2. I am convinced that the PR approval process works well and and will recommend anyone that has a real / legitimate need to have a change merged, to have the discussion.

Thank you very much to Roger and Stuart as well as to @TheShermanTanker ! I will close this.