RFR: 6356745: (coll) Add PriorityQueue(Collection, Comparator) [v2]

[Valeh Hajiyev]

On Wed, 20 Dec 2023 05:05:09 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> @liach thanks for the help. I updated the PR title, also your proposed CSR content looks good to me. would you mind creating it with your proposed content?
>
> @valeh
> 
> Hello, thanks for contributing this change. I can sponsor it.
> 
> Changes to the JDK such as this one require tests to be included. You can see some PriorityQueue tests in test/jdk/java/util/PriorityQueue. However, it looks like these are tests for specific features and bugfixes. A comprehensive set of tests resides in test/jdk/java/util/concurrent/tck/PriorityQueueTest.java. It might be best to add a test case for the new constructor there.
> 
> In addition, changes such as this will require a release note. I've added the `release-note=yes` label to the bug report to indicate this. The release note process is documented in the Developer's Guide here:
> 
> https://openjdk.org/guide/#release-notes
> 
> @liach 
> 
> Thanks for starting the CSR draft. It looks straightforward enough so far; a couple quick comments. 1) It's not necessary to include the implementation in the CSR, just the specification (or specification changes or diffs) and the method signature. 2) You might want to wait until the spec wording settles down before creating the CSR, otherwise you'll have to keep them in sync continually.
> 
> --
> 
> In passing, I note this Stack Overflow answer in response to a question about the lack of a PriorityQueue(Collection, Comparator) constructor:
> 
> https://stackoverflow.com/questions/66170496/java-priority-queue-build-heap-process-time-complexity/66174529#66174529
> 
> In the discussion of this bug report, I think we believe that constructing a PQ with the new constructor should be faster than constructing one with a Comparator and then calling addAll(), but this SO answer seems to indicate that this new constructor isn't necessary. I'm kind of skeptical of this, though. It would be good to have some confirmation that the new constructor indeed provides a performance improvement. Note however that benchmarking can easily turn into a distraction and a time sink, so I don't think a benchmark is required for this change. However, if somebody wants to try something out, please do so. If suitable, a benchmark could be added somewhere in the test/micro hierarchy (possibly as part of a different PR).
> 
> Finally, please note that with the holiday season, I'm planning to take several days away from work, so I might not respond quickly, but I eventually will.

@stuart-marks thanks for the suggestions, I also added the tests as you suggested

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-1870705915