jdk
jdk copied to clipboard
openjdk
8331291: java.lang.classfile.Attributes class performs a lot of static initializations
Hi,
During performance optimization work on Class-File API as JDK lambda generator we found some static initialization killers.
One of them is java.lang.classfile.Attributes with tens of static fields initialized with individual attribute mappers, and common set of all mappers, and static map from attribute names to the mappers.
I propose to turn all the static fields into lazy-initialized static methods and remove PREDEFINED_ATTRIBUTES and standardAttribute(Utf8Entry name) static mapping method from the Attributes API class.
Please let me know your comments or objections and please review the PR and CSR, so we can make it into 23.
Thank you, Adam
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change requires CSR request JDK-8331414 to be approved
Warning
⚠️ Found leading lowercase letter in issue title for 8331291: java.lang.classfile.Attributes class performs a lot of static initializations
Issues
- JDK-8331291: java.lang.classfile.Attributes class performs a lot of static initializations (Enhancement - P4)
- JDK-8331414: java.lang.classfile.Attributes class performs a lot of static initializations (CSR)
Reviewers
- Chen Liang (@liach - Author) ⚠️ Review applies to fd8da774
- Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to fd8da774
- Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to dcbaae85
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19006/head:pull/19006
$ git checkout pull/19006
Update a local copy of the PR:
$ git checkout pull/19006
$ git pull https://git.openjdk.org/jdk.git pull/19006/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19006
View PR using the GUI difftool:
$ git pr show -t 19006
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19006.diff
Webrev
:wave: Welcome back asotona! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@asotona 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:
8331291: java.lang.classfile.Attributes class performs a lot of static initializations
Reviewed-by: liach, redestad, vromero
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 5 new commits pushed to the master branch:
- 0c934ff4e2fb53a72ad25a080d956745a5649f9b: 8325520: Vector loads and stores with indices and masks incorrectly compiled
- c099f14f07260713229cffbe7d23aa8305415a67: 8305457: Implement java.io.IO
- 6a35311468222f9335b43d548df2ecb80746b389: 8241550: [macOS] SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use"
- f16265d69b09640b972b7494ad57158dbdc426bb: 8332226: "Invalid package name:" from source launcher
- 5a2ba952b120394d7cc0d0890619780c1c27a078: 8325841: Remove unused references to vmSymbols.hpp
Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
@asotona The following labels will be automatically applied to this pull request:
compilercore-libshotspot-compiler
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.
Nice changes! Remarks
- The
INSTANCEfields should be declaredfinal, still safe for lazy initialization. - Not from this patch, but the
AttributeStability stability()overrides can be moved toAbstractAttributeMapper, stored in a field, to decrease source code size. AbstractAttributeMappermight become sealed now that its implementations are no longer anonymous, might move it to be a private nested class ofAttributesto omit the long list of permits.
Nice changes! Remarks
- The
INSTANCEfields should be declaredfinal, still safe for lazy initialization.- Not from this patch, but the
AttributeStability stability()overrides can be moved toAbstractAttributeMapper, stored in a field, to decrease source code size.AbstractAttributeMappermight become sealed now that its implementations are no longer anonymous, might move it to be a private nested class ofAttributesto omit the long list of permits.
Yes, that is part of the plan for today :)
Webrevs
- 10: Full - Incremental (db73c2dd)
- 09: Full (37f7f63f)
- 08: Full - Incremental (21515ec2)
- 07: Full - Incremental (b4203cfd)
- 06: Full - Incremental (dcbaae85)
- 05: Full - Incremental (a1a55d71)
- 04: Full (497dd533)
- 03: Full - Incremental (fd8da774)
- 02: Full - Incremental (f0d9174e)
- 01: Full - Incremental (27238368)
- 00: Full (b7b35c5d)
Also the final modifier addition should be included in the CSR, as it changes the access modifiers even if it has no real impact.
Also the
finalmodifier addition should be included in the CSR, as it changes the access modifiers even if it has no real impact.
The class already had only private constructor, so there was no way to extend it.
Yeah, it has no real usage impact but such changes do require CSRs, like https://bugs.openjdk.org/browse/JDK-8305158 for a final on Arrays class, so you should include the final modifier change in your CSR's specdiff.
@asotona this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout JDK-8331291-attributes
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
On a side note, will we update JEP 466 to include this patch?
I hope so, if we get it into 23 ;)
FWIW code changes looks good to me. There seems to be a number of tests that still need to be updated to use the new methods instead of the old constants.
Thank you! Yes, I'm cleaning the tests right now.
Going to push as commit cfdc64fcb43e3b261dddc6cc6947235a9e76154e.
Since your change was applied there have been 5 commits pushed to the master branch:
- 0c934ff4e2fb53a72ad25a080d956745a5649f9b: 8325520: Vector loads and stores with indices and masks incorrectly compiled
- c099f14f07260713229cffbe7d23aa8305415a67: 8305457: Implement java.io.IO
- 6a35311468222f9335b43d548df2ecb80746b389: 8241550: [macOS] SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use"
- f16265d69b09640b972b7494ad57158dbdc426bb: 8332226: "Invalid package name:" from source launcher
- 5a2ba952b120394d7cc0d0890619780c1c27a078: 8325841: Remove unused references to vmSymbols.hpp
Your commit was automatically rebased without conflicts.
@asotona Pushed as commit cfdc64fcb43e3b261dddc6cc6947235a9e76154e.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.