8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize #22255

rkennke · 2024-11-19T20:07:38Z

From the bug description:
ObjLayout::initialize() is called in Arguments::parse(const JavaVMInitArgs*) which sets ObjLayout::_klass_mode. FullGCForwarding::initialize_flags(size_t) is called in init_globals() which seems to be later in the Threads::create_vm(JavaVMInitArgs*, bool*) routine. The latter, however, can unset the UseCompactObjectHeaders flag, which leads to a potential mismatch with ObjLayout::_klass_mode, firing the asserts in ObjLayout::klass_mode().

There is also a related (and somewhat minor) problem: In Arguments::parse() we enable a bunch of stuff when UseCompactObjectHeaders is on (LW locking, but that's the default anyway, and object-monitor-tables, which are otherwise off), but then may disable UseCompactObjectHeaders, e.g. in the GC or when -UseCompressedClassPointers is requested. This then leaves the odd situation that we run without compact headers, but have object monitor tables turned on.

The fix to both issues is:

First disable UCOH
and do that in apply_ergo() (e.g. GCs should do it in GCArguments::initialize())
then enable any flags required by compact headers
Initialize ObjLayout after all flags are done (it's in the correct place, already)

Testing:

tier1 -UCOH (default)
tier1 +UCOH
Manual testing several flag combinations

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-8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize (Bug - P3)

Reviewers

Stefan Karlsson (@stefank - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer)
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22255

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…::initialize

bridgekeeper · 2024-11-19T20:08:46Z

👋 Welcome back rkennke! 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-11-19T20:09:24Z

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

8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize

Reviewed-by: stefank, shade, ayang

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

965aace: 8335468: [XWayland] JavaFX hangs when calling java.awt.Robot.getPixelColor
811d08c: 8340010: Fix vectorization tests with compact headers
519bb26: 8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration
d112f35: 8344855: Remove calls to SecurityManager and doPrivileged in HTTP related implementation classes in the sun.net and sun.net.www.http packages after JEP 486 integration
da4b7a8: 8341436: containers/docker/TestJcmdWithSideCar.java takes needlessly long to run
333a997: 8335231: [macos] Test java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java failed on macOS because the case didn't get the expected PrintAbortException
9576546: 8343698: Linux x86_64 lto build gives a lot of warnings and fails lto-wrapper: fatal error: make returned 2 exit status
68ba7ee: 8340205: Native linker allows MemoryLayout consisting of only PaddingLayout
6f622da: 8344923: Problem list on java/awt/Robot/ScreenCaptureRobotTest.java on macOS
8f08020: 8344903: Improve error handling TestJhsdbJstackPrintVMLocks.java
... and 99 more: https://git.openjdk.org/jdk/compare/3328b4ecf225f95edfce6ab848dcfb62ddc1aaff...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-11-19T20:09:59Z

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

hotspot
shenandoah

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.

rkennke · 2024-11-20T11:23:31Z

/label remove hotspot

rkennke · 2024-11-20T11:23:41Z

/label add hotspot-runtime

openjdk · 2024-11-20T11:25:06Z

@rkennke
The hotspot label was successfully removed.

openjdk · 2024-11-20T11:25:47Z

@rkennke
The hotspot-runtime label was successfully added.

mlbridge · 2024-11-20T11:30:08Z

Webrevs

stefank

I think this looks good. I ran it through gdb with awatch to see that I there was no obvious reads of the variables before they were set.

A couple of small nits that would be nice fix.

src/hotspot/share/gc/serial/serialArguments.cpp

src/hotspot/share/gc/serial/serialArguments.hpp

shipilev

So we are inlining initialize_heap_flags_and_sizes into initialize, right? Is ParallelArguments::initialize_heap_flags_and_sizes() dangling now?

It sounds like there is a little bit of symmetry between CompressedKlassPointers::initialize that is called in universe_init, and thus after argument adjustments. I don't know if it makes sense to move ObjLayout::initialize to universe_init?

shipilev · 2024-11-20T15:14:28Z

src/hotspot/share/runtime/arguments.hpp

@@ -264,6 +264,7 @@ class Arguments : AllStatic {
  static void set_conservative_max_heap_alignment();
  static void set_use_compressed_oops();
  static jint set_ergonomics_flags();
+  static void setup_compact_headers();


Consistency with adjacent methods: should it be called set_compact_headers?

rkennke · 2024-11-20T19:45:11Z

So we are inlining initialize_heap_flags_and_sizes into initialize, right?

No, not really. initialize_heap_flags_and_sizes is still called from initialize_heap_sizes which is called from universe_init.

Is ParallelArguments::initialize_heap_flags_and_sizes() dangling now?

No.

It sounds like there is a little bit of symmetry between CompressedKlassPointers::initialize that is called from universe_init, and thus after argument adjustments. I don't know if it makes sense to move ObjLayout::initialize to universe_init?

Hmm yeah, that makes sense. I'll do that.

stefank

Looks good.

I would prefer though if ObjLayout::initialize moved up to above adjust_heap_sizes_for_dumping, so that adjust_heap_sizes_for_dumping and initialize_heap_sizes continue to be kept together.

shipilev

OK, this makes sense. But it is hairy, so maybe we need someone else to take a look.

stefank · 2024-11-21T20:13:36Z

/label add hotspot-gc

openjdk · 2024-11-21T20:14:10Z

@stefank
The hotspot-gc label was successfully added.

stefank · 2024-11-22T11:19:53Z

Ping hotspot-gc.

src/hotspot/share/runtime/arguments.cpp

shipilev

Still good.

rkennke · 2024-11-25T13:47:09Z

Thanks!

/integrate

openjdk · 2024-11-25T13:48:06Z

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

c6b14c6: 8344841: ClassPrinter prints confusing value for null
ddc8a9d: 8344366: Remove Security Manager dependencies from javax.net.ssl and sun.security.ssl packages
965aace: 8335468: [XWayland] JavaFX hangs when calling java.awt.Robot.getPixelColor
811d08c: 8340010: Fix vectorization tests with compact headers
519bb26: 8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration
d112f35: 8344855: Remove calls to SecurityManager and doPrivileged in HTTP related implementation classes in the sun.net and sun.net.www.http packages after JEP 486 integration
da4b7a8: 8341436: containers/docker/TestJcmdWithSideCar.java takes needlessly long to run
333a997: 8335231: [macos] Test java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java failed on macOS because the case didn't get the expected PrintAbortException
9576546: 8343698: Linux x86_64 lto build gives a lot of warnings and fails lto-wrapper: fatal error: make returned 2 exit status
68ba7ee: 8340205: Native linker allows MemoryLayout consisting of only PaddingLayout
... and 101 more: https://git.openjdk.org/jdk/compare/3328b4ecf225f95edfce6ab848dcfb62ddc1aaff...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-25T13:48:17Z

@rkennke Pushed as commit cb1c736.

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

8344363: FullGCForwarding::initialize_flags is called after ObjLayout…

26c3680

…::initialize

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Nov 19, 2024

rkennke marked this pull request as ready for review November 20, 2024 11:24

openjdk bot added rfr Pull request is ready for review and removed hotspot hotspot-dev@openjdk.org labels Nov 20, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Nov 20, 2024

stefank approved these changes Nov 20, 2024

View reviewed changes

src/hotspot/share/gc/serial/serialArguments.cpp Outdated Show resolved Hide resolved

src/hotspot/share/gc/serial/serialArguments.hpp Show resolved Hide resolved

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

shipilev reviewed Nov 20, 2024

View reviewed changes

Address review comments

6f8b5ad

openjdk bot removed the ready Pull request is ready to be integrated label Nov 20, 2024

Rename setup_compact_headers

a42d490

stefank approved these changes Nov 21, 2024

View reviewed changes

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

Move ObjLayout::initialize() up a little

43c7668

openjdk bot removed the ready Pull request is ready to be integrated label Nov 21, 2024

stefank approved these changes Nov 21, 2024

View reviewed changes

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

shipilev approved these changes Nov 21, 2024

View reviewed changes

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Nov 21, 2024

albertnetymk approved these changes Nov 22, 2024

View reviewed changes

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved

Rename set_compact_headers() -> set_compact_headers_flags()

28a1fa3

openjdk bot removed the ready Pull request is ready to be integrated label Nov 22, 2024

rkennke requested review from albertnetymk, shipilev and stefank November 22, 2024 18:11

stefank approved these changes Nov 22, 2024

View reviewed changes

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

shipilev approved these changes Nov 25, 2024

View reviewed changes

albertnetymk approved these changes Nov 25, 2024

View reviewed changes

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

openjdk bot closed this Nov 25, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize #22255

8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize #22255

rkennke commented Nov 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 19, 2024

openjdk bot commented Nov 19, 2024 •

edited

Loading

openjdk bot commented Nov 19, 2024

rkennke commented Nov 20, 2024

rkennke commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

mlbridge bot commented Nov 20, 2024 •

edited

Loading

stefank left a comment

shipilev left a comment •

edited

Loading

shipilev Nov 20, 2024

rkennke commented Nov 20, 2024

stefank left a comment

shipilev left a comment

stefank commented Nov 21, 2024

openjdk bot commented Nov 21, 2024

stefank commented Nov 22, 2024

shipilev left a comment

rkennke commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize #22255

8344363: FullGCForwarding::initialize_flags is called after ObjLayout::initialize #22255

Conversation

rkennke commented Nov 19, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 19, 2024

openjdk bot commented Nov 19, 2024 • edited Loading

openjdk bot commented Nov 19, 2024

rkennke commented Nov 20, 2024

rkennke commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

openjdk bot commented Nov 20, 2024

mlbridge bot commented Nov 20, 2024 • edited Loading

Webrevs

stefank left a comment

Choose a reason for hiding this comment

shipilev left a comment • edited Loading

Choose a reason for hiding this comment

shipilev Nov 20, 2024

Choose a reason for hiding this comment

rkennke commented Nov 20, 2024

stefank left a comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

stefank commented Nov 21, 2024

openjdk bot commented Nov 21, 2024

stefank commented Nov 22, 2024

shipilev left a comment

Choose a reason for hiding this comment

rkennke commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

rkennke commented Nov 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 19, 2024 •

edited

Loading

mlbridge bot commented Nov 20, 2024 •

edited

Loading

shipilev left a comment •

edited

Loading