8306851: Move Method access flags #13654

coleenp · 2023-04-25T19:09:23Z

This change moves the flags from AccessFlags to either ConstMethodFlags or MethodFlags, depending on whether they are set at class file parse time, which makes them essentially const, or at runtime, which makes them needing atomic access.

This leaves AccessFlags int size because Klass still has JVM flags that are more work to move, but this change doesn't increase Method size. I didn't remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are several of these in other places, and with this change the code is benign.

Tested with tier1-6, and some manual verification of printing.

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-8306851: Move Method access flags

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to 4b92aacf
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 6687cc0e
Doug Simon (@dougxc - Committer) ⚠️ Review applies to 6687cc0e
Matias Saavedra Silva (@matias9927 - Committer) ⚠️ Review applies to 9b05ab84
Frederic Parain (@fparain - Reviewer) ⚠️ Review applies to 6687cc0e

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13654

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-04-25T19:10:06Z

👋 Welcome back coleenp! 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 · 2023-04-25T19:12:20Z

@coleenp The following labels will be automatically applied to this pull request:

hotspot
serviceability

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.

openjdk · 2023-04-26T15:55:02Z

@coleenp Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2023-04-26T16:06:23Z

Webrevs

plummercj

The SA changes look good. I think these changes make @iklam's #13663 fix unnecessary, but harmless.

openjdk · 2023-04-26T17:53:58Z

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

8306851: Move Method access flags

Reviewed-by: cjplummer, dholmes, dnsimon, matsaave, fparain

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

05af487: 8306681: Open source more AWT DnD related tests
ec5c792: 8306133: Open source few AWT Drag & Drop related tests
6d6f726: 8307078: Opensource and clean up five more AWT Focus related tests
89711f3: 8307079: Update test java/awt/Choice/DragOffNoSelect.java
9ff5550: 8307077: Convert CRLF to LF in java.xml.crypto
f83e730: 8302983: ZoneRulesProvider.registerProvider() twice will remove provider
82ccfe3: 8305975: Add TWCA Global Root CA
a177152: 8306331: assert((cnt > 0.0f) && (prob > 0.0f)) failed: Bad frequency assignment in if
e119658: 8303475: potential null pointer dereference in filemap.cpp
cf799ba: 8306950: [REDO] JDK-8305252 make_method_handle_intrinsic may call java code under a lock
... and 55 more: https://git.openjdk.org/jdk/compare/00b1eacad6ae2d5ea5afb1de506768e9ab960743...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.

coleenp · 2023-04-26T19:40:26Z

Thanks Chris. We were wondering what to do with JVM_RECOGNIZED_METHOD_MODIFIERS but we'll clean them up in another pass.

dholmes-ora

General idea is good but I have some issues with naming inconsistencies. A few other queries.

Thanks.

src/hotspot/share/classfile/classFileParser.cpp

src/hotspot/share/oops/constMethodFlags.hpp

src/hotspot/share/oops/method.cpp

src/hotspot/share/oops/method.hpp

coleenp · 2023-04-27T16:14:17Z

@dougxc can you look at the JVMCI changes?

dholmes-ora

Thanks for the updates. I understand about the fanout from making is naming fully consistent.

src/hotspot/share/oops/method.hpp

dougxc · 2023-04-28T07:22:32Z

Thankfully all these changes only impact values read by JVMCI Java code and none in Graal Java code. Looks good to me.

matias9927

Nice change! Just some small nits but it otherwise looks good.

src/hotspot/share/oops/method.hpp

fparain

Thank you for all those cleanings!
Looks good to me.

src/hotspot/share/oops/constMethod.cpp

matias9927

Looks good, thanks!

coleenp · 2023-05-01T11:32:06Z

Thanks David, Chris, Doug, Matias and Fred for reviewing.
/integrate

openjdk · 2023-05-01T11:33:33Z

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

a6b4f25: 8306825: Monitor deflation might be accidentally disabled by zero intervals
2d7c507: 8305778: javax/swing/JTableHeader/6884066/bug6884066.java: Unexpected header's value; index = 4 value = E
e1b06ea: 8305780: javax/swing/JTable/7068740/bug7068740.java fails on Ubunutu 20.04
b54c4a3: 8299713: Test javax/swing/JTableHeader/6889007/bug6889007.java failed: Wrong type of cursor
b3dbf28: 8292275: javac does not emit SYNTHETIC and MANDATED flags for parameters by default
6d6d00b: 8306954: Open source five Focus related tests
bb7608b: 8307088: Allow the jdbc.drivers system property to be searchable
a2d3fc8: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names
d43a5a2: 8307135: java/awt/dnd/NotReallySerializableTest/NotReallySerializableTest.java failed
1f68924: 8306955: Open source several JComboBox jtreg tests
... and 68 more: https://git.openjdk.org/jdk/compare/00b1eacad6ae2d5ea5afb1de506768e9ab960743...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-01T11:33:45Z

@coleenp Pushed as commit 316d303.

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

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 25, 2023

8306851: Move Method access flags

e518338

coleenp marked this pull request as ready for review April 26, 2023 15:49

fix comment

4b92aac

coleenp force-pushed the method-flags branch from 172545d to 4b92aac Compare April 26, 2023 15:52

openjdk bot added the rfr Pull request is ready for review label Apr 26, 2023

plummercj approved these changes Apr 26, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 26, 2023

dholmes-ora reviewed Apr 27, 2023

View reviewed changes

coleenp added 2 commits April 27, 2023 12:39

Add is prefixes and some cleanups.

fc5bcaa

Remove bool argument from ConstMethodFlags.set function.

6687cc0

dholmes-ora approved these changes Apr 28, 2023

View reviewed changes

src/hotspot/share/oops/method.hpp Outdated Show resolved Hide resolved

dougxc approved these changes Apr 28, 2023

View reviewed changes

matias9927 suggested changes Apr 28, 2023

View reviewed changes

src/hotspot/share/oops/method.hpp Outdated Show resolved Hide resolved

src/hotspot/share/oops/method.hpp Show resolved Hide resolved

Updates

9b05ab8

fparain approved these changes Apr 28, 2023

View reviewed changes

src/hotspot/share/oops/constMethod.cpp Outdated Show resolved Hide resolved

matias9927 approved these changes Apr 28, 2023

View reviewed changes

Fix constMethod printing.

d0de72a

openjdk bot added the integrated Pull request has been integrated label May 1, 2023

openjdk bot closed this May 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 1, 2023

coleenp deleted the method-flags branch May 1, 2023 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8306851: Move Method access flags #13654

8306851: Move Method access flags #13654

coleenp commented Apr 25, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 25, 2023

openjdk bot commented Apr 25, 2023

openjdk bot commented Apr 26, 2023

mlbridge bot commented Apr 26, 2023 •

edited

Loading

plummercj left a comment

openjdk bot commented Apr 26, 2023 •

edited

Loading

coleenp commented Apr 26, 2023

dholmes-ora left a comment

coleenp commented Apr 27, 2023

dholmes-ora left a comment

dougxc commented Apr 28, 2023

matias9927 left a comment

fparain left a comment

matias9927 left a comment

coleenp commented May 1, 2023

openjdk bot commented May 1, 2023

openjdk bot commented May 1, 2023

8306851: Move Method access flags #13654

8306851: Move Method access flags #13654

Conversation

coleenp commented Apr 25, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 25, 2023

openjdk bot commented Apr 25, 2023

openjdk bot commented Apr 26, 2023

mlbridge bot commented Apr 26, 2023 • edited Loading

Webrevs

plummercj left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 26, 2023 • edited Loading

coleenp commented Apr 26, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Apr 27, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

dougxc commented Apr 28, 2023

matias9927 left a comment

Choose a reason for hiding this comment

fparain left a comment

Choose a reason for hiding this comment

matias9927 left a comment

Choose a reason for hiding this comment

coleenp commented May 1, 2023

openjdk bot commented May 1, 2023

openjdk bot commented May 1, 2023

coleenp commented Apr 25, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 26, 2023 •

edited

Loading

openjdk bot commented Apr 26, 2023 •

edited

Loading