8331087: Move immutable nmethod data from CodeCache #18984

vnkozlov · 2024-04-26T21:16:03Z

Move immutable nmethod's data from CodeCache to C heap. It includes dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, speculations. It amounts for about 30% (optimized VM) of space in CodeCache.

Use HotSpot's os::malloc() to allocate memory in C heap for immutable nmethod's data. Call vm_exit_out_of_memory() if allocation failed.

Shuffle fields order and change some fields size from 4 to 2 bytes to avoid nmethod's header size increase.

Tested tier1-5, stress,xcomp

Our performance testing does not show difference.

Example of updated -XX:+PrintNMethodStatistics output is in JBS comment.

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-8331087: Move immutable nmethod data from CodeCache (Enhancement - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Dean Long (@dean-long - Reviewer)
Doug Simon (@dougxc - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18984

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-26T21:16:30Z

👋 Welcome back kvn! 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-04-26T21:17:12Z

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

8331087: Move immutable nmethod data from CodeCache

Reviewed-by: thartmann, dlong, dnsimon

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

7272939: 8331200: Serial: Remove unused methods in SerialHeap
70d3f22: 8331175: Parallel: Remove VerifyRememberedSets
549bc6a: 8330685: ZGC: share barrier spilling logic
76cda7b: 8331118: Remove Serial includes from space.hpp
766d0b0: 8330960: Serial: Remove SerialFullGC::_total_invocations
4edac34: 8331167: UBSan enabled build fails in adlc on macOS
c615c18: 8330158: C2: Loop strip mining uses ABS with min int
fb63cba: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
4e5c25e: 8331108: Unused Math.abs call in java.lang.FdLibm.Expm1#compute
16c7dcd: 8330523: Reduce runtime and improve efficiency of KeepAliveTest
... and 4 more: https://git.openjdk.org/jdk/compare/aee91fbc70df716b96c202511b4ff1b302df8d60...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-04-26T21:17:48Z

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

graal
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.

mlbridge · 2024-04-26T21:20:54Z

Webrevs

01: Full - Incremental (1824f46c)
00: Full (6b1f69d9)

vnkozlov · 2024-04-26T21:30:07Z

test/hotspot/jtreg/compiler/c1/TestLinearScanOrderMain.java

 * @bug 8207355
 * @compile TestLinearScanOrder.jasm
 * @run main/othervm -Xcomp -XX:+TieredCompilation -XX:TieredStopAtLevel=1
+ *                   -XX:+IgnoreUnrecognizedVMOptions -XX:NMethodSizeLimit=655360


This test caught one check_cast<> issue during development but only on aarch64.
On x64 the test bailed out compilation before that because default NMethodSizeLimit was not big enough ((64*K)*wordSize = 524288).

vnkozlov · 2024-04-26T21:31:16Z

src/hotspot/share/code/nmethod.hpp

+  int consts_size        () const { return int(          consts_end       () -           consts_begin       ()); }
+  int insts_size         () const { return int(          insts_end        () -           insts_begin        ()); }
+  int stub_size          () const { return int(          stub_end         () -           stub_begin         ()); }
+  int oops_size          () const { return int((address) oops_end         () - (address) oops_begin         ()); }
+  int metadata_size      () const { return int((address) metadata_end     () - (address) metadata_begin     ()); }
+  int scopes_data_size   () const { return int(          scopes_data_end  () -           scopes_data_begin  ()); }
+  int scopes_pcs_size    () const { return int((intptr_t)scopes_pcs_end   () - (intptr_t)scopes_pcs_begin   ()); }
+  int dependencies_size  () const { return int(          dependencies_end () -           dependencies_begin ()); }
+  int handler_table_size () const { return int(          handler_table_end() -           handler_table_begin()); }
+  int nul_chk_table_size () const { return int(          nul_chk_table_end() -           nul_chk_table_begin()); }


Shift by one space to aline code.

src/hotspot/share/code/nmethod.cpp

vnkozlov · 2024-04-26T21:43:53Z

src/hotspot/share/code/nmethod.cpp

+    assert((data_offset() + data_end_offset) <= nmethod_size, "wrong nmethod's size: %d < %d", nmethod_size, (data_offset() + data_end_offset));
+
+    // native wrapper does not have read-only data but we need unique not null address
+    _immutable_data          = data_end();


I can't use nullptr because VM expects not null address when it checks, for example, dependencies_begin() even so sizes are 0.
I used data_end() instead of nullptr in other places too.

src/hotspot/share/code/nmethod.cpp

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java

vnkozlov · 2024-04-27T01:12:54Z

Thank you, @dean-long, for review. I will collect (hopefully) more comments for next update before testing and pushing it.

dougxc · 2024-04-27T20:48:38Z

src/hotspot/share/code/nmethod.hpp

+  enum ResultStatus {
+    passed,
+    code_cache_full,
+    out_of_memory


Maybe out_of_c_heap_memory would be clearer? Or out_of_immutable_data_memory if immutable data may not always be malloc'ed.

May be no_space_for_immutable_data.

dougxc · 2024-04-27T20:57:09Z

src/hotspot/share/jvmci/jvmciRuntime.cpp

                                                        nmethod_entry_patch_offset,
                                                        nmethod_mirror_name,
                                                        failed_speculations);
+      nmethod::ResultStatus result_status;


Please propagate the new out_of_memory result throughout JVMCI (e.g. in JVMCI::CodeInstallResult enum and HotSpotVMConfig.getCodeInstallResultDescription method).

vnkozlov · 2024-04-28T02:33:02Z

@dean-long and @dougxc

I am thinking may be I should not bailout when malloc (or other space reservation in a future) failed to allocate memory for immutable data. But instead increase nmethod size and put immutable data there (as before). Then we bailout only when CodeCache is full as before and we don't need out_of_memory failure reason. May be only record that in logs (when they are enabled).

What do you think?

dean-long · 2024-04-28T07:02:40Z

It only makes sense if the immutable data heap is not also used for other critical resources. If malloc or metaspace were used as the immutable data heap, normally failures in those heaps are fatal, because other critical resources (monitors, classes, etc) are allocated from there, so any failure means the JVM is about to die. There's no reason to find a fall-back method to allocate a new nmethod in that case.

dougxc · 2024-04-28T10:32:49Z

Just to be clear @dean-long , you're saying failure to allocate immutable data in the C heap should result in a fatal error? Makes sense to me as the VM must indeed be very close to crashing anyway in that case. It also, obviates the need for propagating out_of_memory_error to JVMCI code.

vnkozlov · 2024-04-28T23:42:11Z

Update:

Addressed @dean-long first comments.
Based on discussion with Doug and Tom (see comments in JDK-8331087), moved jvmci_data back to nmethod's mutable data section.
Replaced my allocation failure handling code with call to vm_exit_out_of_memory().

I verified (with UseNewCode hack`) that out of memory is correctly reported in fastdebug and product VMs:

#
# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (malloc) failed to allocate 64 bytes. Error detail: nmethod: no space for immutable data
# An error report file with more information is saved as:
# /scratch/kvn/jdk_git/hs_err_pid4086275.log

TobiHartmann

Looks good to me. Did you measure any impact on performance (potentially due to improved code density)?

What's left for JDK-7072317?

I wonder if the CHECKED_CAST changes shouldn't go into a separate RFE.

dean-long · 2024-04-29T07:48:41Z

Just to be clear @dean-long , you're saying failure to allocate immutable data in the C heap should result in a fatal error? Makes sense to me as the VM must indeed be very close to crashing anyway in that case. It also, obviates the need for propagating out_of_memory_error to JVMCI code.

I hadn't thought it through that far, actually. I was only pointing out that the proposed fall-back:

increase nmethod size and put immutable data there (as before).

isn't worth the trouble. But making the C heap failure fatal immediately is reasonable, especially if it simplifies JVMCI error reporting.

dougxc · 2024-04-29T08:50:14Z

JVMCI changes now look good to me.

vnkozlov · 2024-04-29T14:51:16Z

Looks good to me. Did you measure any impact on performance (potentially due to improved code density)?

Thank you, @TobiHartmann, for review.

What's left for JDK-7072317?

Make Relocation info (10% of nmethod size) immutable by moving all encoded pointers (external words and others, which we need to patch in Leyden when loading cached code) from it into separate mutable section.

I wonder if the CHECKED_CAST changes shouldn't go into a separate RFE.

No, I want clear information which cast failed instead of trying to reproduce very intermittent failure like this: JDK-8331253. When you have several checked_cast<T> in one method it is impossible to find which failed without this macro.

vnkozlov · 2024-04-29T15:57:11Z

Thank you, Dean, Doug and Tobias for reviews.

vnkozlov · 2024-04-29T15:57:17Z

/integrate

openjdk · 2024-04-29T15:58:04Z

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

8b8fb64: 8324776: runtime/os/TestTransparentHugePageUsage.java fails with The usage of THP is not enough
151ef5d: 8330677: Add Per-Compilation memory usage to JFR
7272939: 8331200: Serial: Remove unused methods in SerialHeap
70d3f22: 8331175: Parallel: Remove VerifyRememberedSets
549bc6a: 8330685: ZGC: share barrier spilling logic
76cda7b: 8331118: Remove Serial includes from space.hpp
766d0b0: 8330960: Serial: Remove SerialFullGC::_total_invocations
4edac34: 8331167: UBSan enabled build fails in adlc on macOS
c615c18: 8330158: C2: Loop strip mining uses ABS with min int
fb63cba: 8330684: ClassFile API runs into StackOverflowError while parsing certain class' bytes
... and 6 more: https://git.openjdk.org/jdk/compare/aee91fbc70df716b96c202511b4ff1b302df8d60...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-29T15:58:14Z

@vnkozlov Pushed as commit bdcc240.

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

8331087: Move immutable nmethod data from CodeCache

6b1f69d

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

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 26, 2024

vnkozlov commented Apr 26, 2024

View reviewed changes

dean-long reviewed Apr 27, 2024

View reviewed changes

src/hotspot/share/code/nmethod.cpp Outdated Show resolved Hide resolved

dean-long reviewed Apr 27, 2024

View reviewed changes

src/hotspot/share/code/nmethod.cpp Outdated Show resolved Hide resolved

dean-long reviewed Apr 27, 2024

View reviewed changes

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java Outdated Show resolved Hide resolved

dougxc reviewed Apr 27, 2024

View reviewed changes

Address comments. Moved jvmci_data back to mutable data section.

1824f46

vnkozlov requested review from dean-long and dougxc April 29, 2024 02:59

TobiHartmann approved these changes Apr 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2024

dean-long approved these changes Apr 29, 2024

View reviewed changes

dougxc approved these changes Apr 29, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 29, 2024

openjdk bot closed this Apr 29, 2024

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

vnkozlov deleted the 8331087 branch April 29, 2024 15:59

bulasevich mentioned this pull request Oct 3, 2024

8343789: Move mutable nmethod data out of CodeCache #21276

Closed

3 tasks

8331087: Move immutable nmethod data from CodeCache #18984

8331087: Move immutable nmethod data from CodeCache #18984

Uh oh!

Conversation

vnkozlov commented Apr 26, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 26, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Apr 26, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

vnkozlov Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

vnkozlov Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vnkozlov Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vnkozlov commented Apr 27, 2024

Uh oh!

dougxc Apr 27, 2024 • 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!

vnkozlov Apr 28, 2024 • 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!

dougxc Apr 27, 2024 • 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!

vnkozlov commented Apr 28, 2024

Uh oh!

dean-long commented Apr 28, 2024

Uh oh!

dougxc commented Apr 28, 2024

Uh oh!

vnkozlov commented Apr 28, 2024

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

dean-long commented Apr 29, 2024

Uh oh!

dougxc commented Apr 29, 2024

Uh oh!

vnkozlov commented Apr 29, 2024

Uh oh!

vnkozlov commented Apr 29, 2024

Uh oh!

vnkozlov commented Apr 29, 2024

Uh oh!

openjdk bot commented Apr 29, 2024

Uh oh!

openjdk bot commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

vnkozlov commented Apr 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 26, 2024 •

edited

Loading

mlbridge bot commented Apr 26, 2024 •

edited

Loading

dougxc Apr 27, 2024 •

edited

Loading

vnkozlov Apr 28, 2024 •

edited

Loading

dougxc Apr 27, 2024 •

edited

Loading