8354111: JavaDoc states that Iterator.remove() is linear in the LinkedBlockingDeque

[kabutz]

One of the features of the LinkedBlockingDeque is that it is a doubly-linked node queue, with pointers in each node to "prev" and "next", which allows remove() in the Iterator to remove the node in constant time. However, in the JavaDoc of the class, it lists Iterator.remove() as an example of a method that takes linear time.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change requires CSR request JDK-8354174 to be approved
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issues

• JDK-8354111: JavaDoc states that Iterator.remove() is linear in the LinkedBlockingDeque (Bug - P4)
• JDK-8354174: JavaDoc states that Iterator.remove() is linear in the LinkedBlockingDeque (CSR)

Reviewers

• Chen Liang (@liach - Reviewer)
• Stuart Marks (@stuart-marks - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24500/head:pull/24500
$ git checkout pull/24500

Update a local copy of the PR:
$ git checkout pull/24500
$ git pull https://git.openjdk.org/jdk.git pull/24500/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24500

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24500.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kabutz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@kabutz This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8354111: JavaDoc states that Iterator.remove() is linear in the LinkedBlockingDeque

Reviewed-by: liach, smarks

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 130 new commits pushed to the master branch:

• 92e52fe: 8353946: Incorrect WINDOWS ifdef in os::build_agent_function_name
• 5280b7b: 8353592: Open source several scrollbar tests
• 9e7763d: 8354536: Problem-list java/util/logging/LoggingDeadlock5.java due to JDK-8354424
• ... and 127 more: https://git.openjdk.org/jdk/compare/fda5eecd6717eb6e1db56be3e41b65deae6e683e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach, @stuart-marks) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@kabutz 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.

[liach]

FYI @kabutz you can log in to bugs.openjdk.org and create an issue for your patch. This issue can be noreg-doc, but will require a small CSR (which you can create and write, as you are already an author) as this touches specification text, for archival purpose.

[kabutz]

Hi @liach I found another mistake in the JavaDoc - it states "Linked nodes are dynamically created upon each insertion unless this would bring the deque above capacity." - however, in the LBD they do create the nodes even if we have reached capacity. This comment was copied over from LBQ, but is not strictly speaking correct for LBD. We could change the code to reflect the comment, or remove that sentence. Which way should we proceed?

[kabutz]

Hi @liach I found another mistake in the JavaDoc - it states "Linked nodes are dynamically created upon each insertion unless this would bring the deque above capacity." - however, in the LBD they do create the nodes even if we have reached capacity. This comment was copied over from LBQ, but is not strictly speaking correct for LBD. We could change the code to reflect the comment, or remove that sentence. Which way should we proceed?

Actually scratch that - I'll propose a fix that seems to improve other issues as well.

[mlbridge]

Webrevs

• 00: Full (917c851a)

[kabutz]

Added https://bugs.openjdk.org/browse/JDK-8354169 - not sure if that is what you meant @liach ?

[liach]

Yep, that's how we can create issues; we already have 8354111, you can close one of them as a duplicate of the other as we will just have one patch.

/csr

[openjdk]

@liach JCheck configuration is invalid in the target branch of this pull request. Please issue this command again once the problem has been resolved.

[kabutz]

Ok, I've filled in the details for the CSR for 8354111. Sorry for all the additional work @liach

[liach]

Thanks! FYI the CSR requires a few more fields to be filled - see https://wiki.openjdk.org/display/csr/Fields+of+a+CSR+Request for details. You can click the "Edit" button on the CSR to see all those fields; many are not available in the browsing view.

In addition, I think you can mention that this constant time is already the preexisting behavior and indicate there is no (minimal, "None" means the field is not filled) compatibility risk.

Once you have filled all required or needed fields (In particular, don't forget about fix version of 25 and scope of SE), I can review the CSR and you can proceed to move it to "Finalized" (Click on the "Draft" button)

[kabutz]

@liach JCheck configuration is invalid in the target branch of this pull request. Please issue this command again once the problem has been resolved.

Any idea what this is about @liach ?

[AlanBateman]

This seems to date from JDK 6 when LBD was added. The list of methods in the class description that don't run in constant time isn't fully exhaustive, but doing a CSR would do no harm here.

[liach]

Any idea what this is about @liach ?

I assume this is a rare glitch in the bot. This PR has the csr label and is in a right state.

[kabutz]

Thanks! FYI the CSR requires a few more fields to be filled - see https://wiki.openjdk.org/display/csr/Fields+of+a+CSR+Request for details. You can click the "Edit" button on the CSR to see all those fields; many are not available in the browsing view.

In addition, I think you can mention that this constant time is already the preexisting behavior and indicate there is no (minimal, "None" means the field is not filled) compatibility risk.

Once you have filled all required or needed fields (In particular, don't forget about fix version of 25 and scope of SE), I can review the CSR and you can proceed to move it to "Finalized" (Click on the "Draft" button)

Please let me know once I can set it to "Finalized". I hope I filled everything in correctly ...

[liach]

Thanks for going through this process; since there is no actual impact on all of source/target/behavioral compatibility you can uncheck all those. I have reviewed the CSR; you can finalize it now.

After CSRs are finalized, the CSR lead (currently Joe Darcy) checks periodically and review the CSRs. It may take a few days.

[kabutz]

Joe Darcy has approved the CSR. Can we go ahead with integrating this PR? We might merge the other 4 PRs into a single one.

[liach]

Looks good, This file has no copyright year so doesn't need a header update. @stuart-marks mind leaving an approval too since you have reviewed the CSR?

[stuart-marks]

LGTM

[kabutz]

/integrate

[openjdk]

@kabutz
Your change (at version 917c851) is now ready to be sponsored by a Committer.

[liach]

Thanks for the fix!

/sponsor

[openjdk]

Going to push as commit c7b70a1.
Since your change was applied there have been 130 commits pushed to the master branch:

• 92e52fe: 8353946: Incorrect WINDOWS ifdef in os::build_agent_function_name
• 5280b7b: 8353592: Open source several scrollbar tests
• 9e7763d: 8354536: Problem-list java/util/logging/LoggingDeadlock5.java due to JDK-8354424
• ... and 127 more: https://git.openjdk.org/jdk/compare/fda5eecd6717eb6e1db56be3e41b65deae6e683e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@liach @kabutz Pushed as commit c7b70a1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.