8336267: Method and Constructor signature parsing can be shared on the root object #20179

liach · 2024-07-15T04:05:55Z

A straightforward optimization, to share the signature parsing of method, constructor, and field between the root and the copied objects, like how method handle accessors are shared.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8336267: Method and Constructor signature parsing can be shared on the root object (Enhancement - P4)

Reviewers

Mandy Chung (@mlchung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20179

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…e root object

bridgekeeper · 2024-07-15T04:06:37Z

👋 Welcome back liach! 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 · 2024-07-15T04:07:06Z

@liach 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:

8336267: Method and Constructor signature parsing can be shared on the root object

Reviewed-by: mchung

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 50 new commits pushed to the master branch:

37a3398: 8343242: RISC-V: Refactor materialization of literal address
d26412e: 8343433: Update net.properties and java.net.http module-info.java after 8326949
29882bf: 8340311: JPackage app-image exe launches multiple exe's in JDK 22+
069bb79: 8342082: Remove unused BasicProgressBarUI.Animator.interval
00ec105: 8343412: Missing escapes for single quote marks in javac.properties
8c1cf8f: 8339128: Cannot resolve user specified tool properly after JDK-8338304
3c7082a: 8343419: Assertion failure in long vector unsigned min/max with -XX:+UseKNLSetting
c82ad84: 8342183: Update tests to use stronger algorithms and keys
1eccdfc: 8343439: [JVMCI] Fix javadoc of Services.getSavedProperties
ea110c3: 8343236: Use @APinote and @implSpec in j.util.Currency
... and 40 more: https://git.openjdk.org/jdk/compare/f7f5198367c74a97933fba7b18aa68f340a5a59c...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-07-15T04:07:37Z

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

mlbridge · 2024-07-15T04:10:19Z

Webrevs

bridgekeeper · 2024-08-12T05:37:54Z

@liach 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!

liach · 2024-08-12T13:51:59Z

Keep alive.

cl4es · 2024-08-12T14:24:51Z

There are a lot of transient volatile fields in this code. Could anything be done to make the various Repository classes more final and less transient/volatile here so that they can be safely published without synchronization, making it more obviously correct that we can safely pick them up from a shared root node (which might more typically be accessed concurrently and thus more exposed to races)?

liach · 2024-08-12T19:15:51Z

Yep, there's #19281 exactly for that purpose: https://github.com/openjdk/jdk/pull/19281/files#diff-5106bd8a6cf3197be7eaee467983c8992981a615bedb66efa50dd97c60cb0041 Uses volatile @Stable instead (or just StableValue once #19625 is ready)

…member-sig-share

bridgekeeper · 2024-09-19T00:26:42Z

@liach 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!

bridgekeeper · 2024-10-17T01:38:14Z

@liach This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

liach · 2024-10-17T01:48:58Z

/open

openjdk · 2024-10-17T01:49:43Z

@liach This pull request is now open

mlchung

It's okay to extend the root object to share genericInfo besides the accessor. But we need to update the comment to make that clear.

    @Stable
    private ConstructorAccessor constructorAccessor;
    // For sharing of ConstructorAccessors. This branching structure
    // is currently only two levels deep (i.e., one root Constructor
    // and potentially many Constructor objects pointing to it.)
    //
    // If this branching structure would ever contain cycles, deadlocks can
    // occur in annotation code.
    private Constructor<T>      root;

The comment for root should be extended to cover the generics repository. In addition, I suggest to move the declaration of instance variable genericInfo closer to the accessor and root.

mlchung · 2024-10-29T22:31:09Z

src/java.base/share/classes/java/lang/reflect/Constructor.java

+                genericInfo =
+                        ConstructorRepository.make(getSignature(),
+                                getFactory());


Suggested change

genericInfo =

ConstructorRepository.make(getSignature(),

getFactory());

genericInfo = ConstructorRepository.make(getSignature(), getFactory());

Nit: This fits well in one line.

mlchung · 2024-10-29T22:32:06Z

src/java.base/share/classes/java/lang/reflect/Field.java

+                genericInfo = FieldRepository.make(getGenericSignature(),
+                        getFactory());


Suggested change

genericInfo = FieldRepository.make(getGenericSignature(),

getFactory());

genericInfo = FieldRepository.make(getGenericSignature(), getFactory());

mlchung · 2024-10-29T22:32:16Z

src/java.base/share/classes/java/lang/reflect/Method.java

+                genericInfo = MethodRepository.make(getGenericSignature(),
+                        getFactory());


Suggested change

genericInfo = MethodRepository.make(getGenericSignature(),

getFactory());

genericInfo = MethodRepository.make(getGenericSignature(), getFactory());

…member-sig-share

liach · 2024-10-30T09:53:49Z

Thanks; I have simplified the existing comments and grouped the lazy shareable fields together.

mlchung

Looks good.

src/java.base/share/classes/java/lang/reflect/Method.java

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

liach · 2024-10-30T16:44:43Z

Applied this minor patch. java/lang/reflect tests are still all fine.

liach · 2024-11-04T13:23:01Z

/integrate

openjdk · 2024-11-04T13:24:33Z

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

1f7d524: 8343437: ClassDesc.of incorrectly permitting empty names
895a7b6: 8342967: Lambda deduplication fails with non-metafactory BSMs and mismatched local variables names
b41d713: 8343513: Forward declare Thread in mutexLocker.hpp
809030b: 8321500: javadoc rejects '@' in multi-line attribute value
7bca0af: 8343128: PassFailJFrame.java test result: Error. Bad action for script: build}
f69b601: 8343188: Investigate ways to simplify MemorySegment::ofBuffer
7f131a9: 8343415: RISC-V: Increase maximum size of C2EntryBarrierStub by four
452a5fb: 8343507: Parallel: Fail if verify_complete finds incorrect states
7580199: 8343205: CompileBroker::possibly_add_compiler_threads excessively polls available memory
df08a9e: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF
... and 54 more: https://git.openjdk.org/jdk/compare/f7f5198367c74a97933fba7b18aa68f340a5a59c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-04T13:24:41Z

@liach Pushed as commit 8d6cfba.

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

8336267: Method and Constructor signature parsing can be shared on th…

31b0bf4

…e root object

openjdk bot added the rfr Pull request is ready for review label Jul 15, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 15, 2024

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

51a7cee

…member-sig-share

bridgekeeper bot closed this Oct 17, 2024

openjdk bot reopened this Oct 17, 2024

mlchung reviewed Oct 29, 2024

View reviewed changes

liach added 2 commits October 30, 2024 03:55

Merge branch 'master' of https://github.com/openjdk/jdk into feature/…

42c193d

…member-sig-share

Review remarks, comment cleanup

8147d66

Missed field

5ff5797

mlchung approved these changes Oct 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2024

mlchung reviewed Oct 30, 2024

View reviewed changes

src/java.base/share/classes/java/lang/reflect/Method.java Outdated Show resolved Hide resolved

Move hash cache out of shared states section

102fc3f

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Oct 30, 2024

mlchung approved these changes Nov 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 4, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 4, 2024

openjdk bot closed this Nov 4, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 4, 2024

		genericInfo = FieldRepository.make(getGenericSignature(),
		getFactory());

		genericInfo = MethodRepository.make(getGenericSignature(),
		getFactory());

8336267: Method and Constructor signature parsing can be shared on the root object #20179

8336267: Method and Constructor signature parsing can be shared on the root object #20179

Uh oh!

Conversation

liach commented Jul 15, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 15, 2024

Uh oh!

openjdk bot commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 15, 2024

Uh oh!

mlbridge bot commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

bridgekeeper bot commented Aug 12, 2024

Uh oh!

liach commented Aug 12, 2024

Uh oh!

cl4es commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach commented Aug 12, 2024

Uh oh!

bridgekeeper bot commented Sep 19, 2024

Uh oh!

bridgekeeper bot commented Oct 17, 2024

Uh oh!

liach commented Oct 17, 2024

Uh oh!

openjdk bot commented Oct 17, 2024

Uh oh!

mlchung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

mlchung Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

mlchung Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

liach commented Oct 30, 2024

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liach commented Oct 30, 2024

Uh oh!

liach commented Nov 4, 2024

Uh oh!

openjdk bot commented Nov 4, 2024

Uh oh!

openjdk bot commented Nov 4, 2024

Uh oh!

Uh oh!

liach commented Jul 15, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 15, 2024 •

edited

Loading

mlbridge bot commented Jul 15, 2024 •

edited

Loading

cl4es commented Aug 12, 2024 •

edited

Loading

mlchung left a comment •

edited

Loading