-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8343039: Remove jdk.internal.misc.InternalLock and usages from java.io #22048
Conversation
👋 Welcome back bpb! A progress list of the required criteria for merging this PR into |
@bplb 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:
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 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Dynamic buffer resizing in The |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/openjdk/jdk/tree/5212535a276a92d96ca20bdcfccfbce956febdb1 is a baseline for before virtual threads were integrated. Note that in this period, there are other optimizations such as lazy allocation for buffer arrays or some jfr changes, so these aren't exact matches.
I think we should remove jdk.internal.misc.InternalLock and the usages in Throwable as part of this change. I don't mind if JavaIOPrintStreamAccess is removed now or in a later PR. |
I will address these items first and then the other comments, but will probably refrain from much in the way of simple cleanups, at least before the final iteration. |
|
Yes, these two will need to be updated. I don't mind if this is split up into several PRs but if InternalLock isn't removed by the current PR then you'll need to change its title :-) |
I'll go ahead and update these and remove InternalLock. |
…able; remove InternalLock class; address comments on BIS and BOS
21c9c5e addresses reviewer comments above and removes |
It is removed by d3d1e3a. |
I did a pass over the latest update, a lot of changes, it's good that Chen is reviewing too as would be too easy to introduce a bug with this refactor. |
Definitely on both counts. |
f5548c1 fixes a test failure caught during CI testing. |
…a/lang/ProcessBuilder/Basic.java
…cessBuilder/Basic.java
/reviewers 2 |
@AlanBateman |
I've changed the PR to require at least two reviewers as I think the changes could benefit from a second reviewer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another pass over this and don't see any issues except the confusing comment in the BOS constructor. Overall good cleanup as InternalLock was a temporary crutch to avoid pinning in the java.io area for common usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Aside from the output stream smaller buffer from virtual threads, everything is pretty much a restoration to JDK18's state (aside from trivial whitespace changes)
ensureOpen(); | ||
boolean omitLF = ignoreLF || skipLF; | ||
if (term != null) term[0] = false; | ||
bufferLoop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we usually handle the indentation of labels? I personally prefer this type of indentation, but in the jdk18 code the label has one less level of indentation, so it aligns with the enclosing }
. Don't know if we are looking for parity with 18 code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the indentation convention for labels is. Neither the draft style guide (https://cr.openjdk.org/~alundblad/styleguide/index-v6.html) nor the ancient code conventions from 1997 mention this.
Thanks, @liach , for the second review. |
/integrate |
Going to push as commit 0b9b82a.
Your commit was automatically rebased without conflicts. |
Uses of
InternalLock
are removed andsynchronized
is reinstated.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22048/head:pull/22048
$ git checkout pull/22048
Update a local copy of the PR:
$ git checkout pull/22048
$ git pull https://git.openjdk.org/jdk.git pull/22048/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22048
View PR using the GUI difftool:
$ git pr show -t 22048
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22048.diff
Using Webrev
Link to Webrev Comment