8058176: [mlvm] tests should not allow code cache exhaustion #2523

lepestock · 2021-02-11T13:27:52Z

Another approach to the JDK-8058176 and #2440 - never allowing the tests hit CodeCache limits. The most significant consumer is the MH graph builder (the MHTransformationGen), whose consumption is now controlled. List of changes:

Code cache size getters are added to WhiteBox;
MH sequences are now built with remaining Code cache size in mind (always let 2M clearance);
Dependencies on WhiteBox added for all affected tests;
The test cases in question un-problemlisted.

Testing: the whole vmTestbase/vm/mlvm/ in win-lin-mac x86.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8058176: [mlvm] tests should not allow code cache exhaustion

Reviewers

Igor Ignatyev (@iignatev - Reviewer) ⚠️ Review applies to 64ae20d

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2523/head:pull/2523
$ git checkout pull/2523

To update a local copy of the PR:
$ git checkout pull/2523
$ git pull https://git.openjdk.java.net/jdk pull/2523/head

bridgekeeper · 2021-02-11T13:28:52Z

👋 Welcome back enikitin! 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 · 2021-02-11T13:29:53Z

@lepestock The following label will be automatically applied to this pull request:

hotspot

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 · 2021-02-11T13:32:46Z

Webrevs

lepestock · 2021-02-12T19:44:58Z

As suggested by @iignatev, cleaned off WhiteBox changes in favour of management JMX beans, resulting in much cleaner solution.

iignatev · 2021-02-12T20:03:01Z

test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/share/MHTransformationGen.java

@@ -63,10 +66,21 @@

    private static final boolean USE_THROW_CATCH = false; // Test bugs

+    private static final MemoryPoolMXBean CODE_CACHE_MX_BEAN = ManagementFactory


does it work w/ both -XX:+SegmentedCodeCache and -XX:-SegmentedCodeCache?
If I remember correctly (@TobiHartmann , please correct me if I'm wrong), CodeCache pool exists when SegmentedCodeCache is disabled, when it's enabled, you will have 3 different pools (one for each "CodeHeap"), and here we would need to use one for non-nmethod codeheap.

-- Igor

Thanks for the info about the segmented code cache. I did some research and found that the opposite is true - both nmethod pools ('profiled' and 'non-profiled') are growing along with the MH graph growth. This is supported by the specification for non-method code heap at:

https://docs.oracle.com/en/java/javase/15/vm/java-hotspot-virtual-machine-performance-enhancements.html#GUID-1D9B26AD-8E0A-4771-90DA-A81A2C1F5B55

Please check the the fixed version.

o/c they grow, b/c we use them for compiled code and if there is no space in non-nmethod heap, we use them for adapters as well, so I guess that the growth that you see is already after non-nmethod heap got exhausted. I'd recommend you simply use the sum of all available code-heaps (this will increase the possibility of false-positive results due to segmentation, but I don't think it matters much here).

Well, seems like rebalancing doesn't works that good. Here's a sample failure with plenty of free space in the non-nmethods heap:

[8.230s][warning][codecache] CodeHeap 'non-profiled nmethods' is full. Compiler has been disabled. [8.230s][warning][codecache] Try increasing the code heap size using -XX:NonProfiledCodeHeapSize= Java HotSpot(TM) 64-Bit Server VM warning: CodeHeap 'non-profiled nmethods' is full. Compiler has been disabled. Java HotSpot(TM) 64-Bit Server VM warning: Try increasing the code heap size using -XX:NonProfiledCodeHeapSize= CodeHeap 'non-profiled nmethods': size=8192Kb used=8191Kb max_used=8191Kb free=0Kb << Exhausted CodeHeap 'profiled nmethods': size=8192Kb used=8191Kb max_used=8191Kb free=0Kb << Exhausted CodeHeap 'non-nmethods': size=102400Kb used=18343Kb max_used=18343Kb free=84056Kb << 84Mb of free space # ERROR: Caught exception in Thread[Thread-41,5,MainThreadGroup] ... # ERROR: Caused by: java.lang.VirtualMachineError: Out of space in CodeCache for method handle intrinsic

The sum monitoring won't help here either. I've added non-nmethods heap to the monitoring, just to be sure.

hm... that can mean that there is a product bug (or my recollections about code heaps aren't as good as I thought).

@TobiHartmann , @iwanowww, could you please take a look? Evgeny's observations suggest that method handle intrinsics use non-profiled nmethods and profiled nmethods heaps and not non-nmethods heap despite the fact that the last one has plenty of free space. my understanding is/was that we should have used non-nmethods heap for MH intrinsic 1st and if it's exhausted start to use the other heaps.

Thanks,
-- Igor

I don't see a compelling reason why method handle linkers have to be nmethods and live in 'profiled'/'non-profiled' code heaps. I think the reason why it works that way now is the linkers are treated as ordinary native wrappers (since linker methods are just signature-polymorphic native static methods declared on java.lang.invoke.MethodHandle class). But native wrappers are represented as nmethods for a reason: they can be unloaded along with the class.
It's not the case with MH linkers which aren't unloaded at all.

Please, file an RFE if you find it desirable to put MH linkers into 'non-nmethods' heap.

Raised the RFE: https://bugs.openjdk.java.net/browse/JDK-8263377

Here's the rationale behind the non-nmethods heap monitoring: the test i2c_c2i that fills up that heap. The test was started with 8MB per each heap, and consumed all the non-nmethods one. The consumption speed diminishes quickly after ~10 MB, but with the limits of 8 MB (stated in the case) we can hit the Error:

Heaps statistics: [non-nm: 1.8 MiB : 22%][profiled: 781.3 KiB : 9%][non-profiled: 189.3 KiB : 2%] ... Heaps statistics: [non-nm: 8.0 MiB : 99%][profiled: 3.0 MiB : 37%][non-profiled: 8.0 MiB : 99%]

Raised the RFE: https://bugs.openjdk.java.net/browse/JDK-8263377

do you plan to change MHTransformationGen:: isCodeCacheEffectivelyFull after/if 8263377 got integrated?

Raised the RFE: https://bugs.openjdk.java.net/browse/JDK-8263377

do you plan to change MHTransformationGen:: isCodeCacheEffectivelyFull after/if 8263377 got integrated?

Raised the https://bugs.openjdk.java.net/browse/JDK-8263398 to not forget about it.

iignatev · 2021-02-12T20:05:55Z

test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/share/MHTransformationGen.java

@@ -88,6 +102,12 @@ public static MHMacroTF createSequence(Argument finalRetVal, Object boundObj, Me
        List<MHTFPair> pendingPWTFs = new LinkedList<MHTFPair>();

        for ( int i = nextInt(MAX_CYCLES); i > 0; i-- ) {
+            if (isCodeCacheEffectivelyFull()) {
+                Env.traceNormal("Not enought code cache to build up MH sequences anymore. " +
+                        " Has only been able to achieve " + (MAX_CYCLES - i) + " out of " + MAX_CYCLES);


given nextInt(x) returns a random number from [0; x], we might have achieved more (or less) MAX_CYCLES - i, i.e. that part of the message is incorrect, I'd just remove it.

Fixed by extracting the generated random number first.

iignatev · 2021-03-10T18:03:44Z

test/hotspot/jtreg/vmTestbase/vm/mlvm/meth/share/MHTransformationGen.java

+        NON_SEGMENTED_CODE_CACHE_POOL.ifPresent(pool -> check.accept(pool, 2_000_000));
+        NON_NMETHODS_POOL.ifPresent(pool -> check.accept(pool, 1_000_000));
+        PROFILED_NMETHODS_POOL.ifPresent(pool -> check.accept(pool, 1_000_000));
+        NON_PROFILED_NMETHODS_POOL.ifPresent(pool -> check.accept(pool, 1_000_000));


could you please introduce a (or two) static final field for these constants and use them here?

could you please introduce a (or two) static final field for these constants and use them here?

Fixed in the new version.

openjdk · 2021-03-16T16:16:37Z

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

8058176: [mlvm] tests should not allow code cache exhaustion

Reviewed-by: iignatyev

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

444a80b: 8263455: NMT: assert on registering a region which completely engulfs an existing region
2b93ae0: 8261480: MetaspaceShared::preload_and_dump should check exceptions
81ba578: 8263676: AArch64: one potential bug in C1 LIRGenerator::generate_address()
9225a23: 8263108: Class initialization deadlock in java.lang.constant
5d5813a: 8263757: Remove serviceability/sa/ClhsdClasses.java from ZGC problem list
50ff0d4: 8263756: Fix ZGC ProblemList entry for serviceability/sa/ClhsdbSymbol.java
99b39aa: 8262807: Note assumptions of core reflection modeling and parameter handling
26234b5: 8254979: Class.getSimpleName() returns non-empty for lambda and method
83a49ef: 8263753: two new tests from JDK-8261671 fail with "Error. can not find ClassFileInstaller in test directory or libraries"
24afa36: 8263726: divideToIntegralValue typo on BigDecimal documentation
... and 6 more: https://git.openjdk.java.net/jdk/compare/000012a3b08d5f21d4cb2369d2122adfae1a3e9b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@iignatev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

lepestock · 2021-03-18T06:25:42Z

/integrate

openjdk · 2021-03-18T06:26:34Z

@lepestock
Your change (at version e76dd09) is now ready to be sponsored by a Committer.

shipilev · 2021-04-01T06:54:21Z

Are we integrating this? I thought to sponsor, but wanted to check if everyone is in agreement this change is fine.

iignatev · 2021-04-01T14:19:00Z

Fine w/ me.
(Don’t know why I haven’t sponsored it before)

iignatev · 2021-04-14T17:22:12Z

hm... I guess no one else wants to share their opinion, so I'll take it as an agreement and /sponsor

iignatev · 2021-04-14T17:31:49Z

/sponsor

openjdk · 2021-04-14T17:33:18Z

@iignatev @lepestock Since your change was applied there have been 411 commits pushed to the master branch:

9406744: 8264976: Minor numeric bug in AbstractSplittableWithBrineGenerator.makeSplitsSpliterator
80026d8: 8265174: Update Class.getDeclaredMethods to discuss synthetic and bridge methods
ffb3771: 8265019: Update tests for additional TestNG test permissions
7e4cd48: 8264821: DirectIOTest fails on a system with large block size
0afcbd4: 8264412: AArch64: CPU description should refer DMI
4661690: 8262883: doccheck: Broken links in java.base
75b039a: 8263970: Manual test javax/swing/JTextField/JapaneseReadingAttributes/JapaneseReadingAttributes.java failed
f5b2f08: 8257836: Add additional test cases to TestSyncOnValueBasedClassEvent.java
27dd88b: 8262957: (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled
e2106d5: 8265104: CpuLoad and SystemCpuLoad in OperatingSystem MXBean returns -1.0
... and 401 more: https://git.openjdk.java.net/jdk/compare/000012a3b08d5f21d4cb2369d2122adfae1a3e9b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4c83d24.

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

lepestock added 3 commits February 11, 2021 14:19

8058176: [mlvm] tests should not allow code cache exhaustion

b620a31

Add CodeCache methods to the WhiteBox

3baaab1

Un-problemlist the OOME tests

4153edb

openjdk bot added the rfr Pull request is ready for review label Feb 11, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 11, 2021

Switch to ManagementBeans approach instead of the WhiteBox one

71af718

iignatev reviewed Feb 12, 2021

View reviewed changes

lepestock added 3 commits February 16, 2021 20:07

Add support for segmented CodeCache

f15a742

Fix 'cycles to build' error output

763d94b

Add non-nmethods pool to the monitoring

6a3c478

iignatev suggested changes Mar 10, 2021

View reviewed changes

lepestock added 2 commits March 11, 2021 22:19

Extract allowances into constants

76b0272

Move a comment a bit higher

64ae20d

iignatev approved these changes Mar 16, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 16, 2021

Merge branch 'master' into JDK-8058176/public

e76dd09

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 18, 2021

openjdk bot closed this Apr 14, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8058176: [mlvm] tests should not allow code cache exhaustion #2523

8058176: [mlvm] tests should not allow code cache exhaustion #2523

lepestock commented Feb 11, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 11, 2021

openjdk bot commented Feb 11, 2021

mlbridge bot commented Feb 11, 2021 •

edited

Loading

lepestock commented Feb 12, 2021

iignatev Feb 12, 2021

lepestock Feb 16, 2021

iignatev Feb 16, 2021

lepestock Feb 17, 2021

iignatev Feb 17, 2021

iwanowww Mar 4, 2021

lepestock Mar 10, 2021

lepestock Mar 10, 2021

iignatev Mar 10, 2021

lepestock Mar 10, 2021

iignatev Feb 12, 2021

lepestock Feb 16, 2021

iignatev Mar 10, 2021

lepestock Mar 11, 2021

openjdk bot commented Mar 16, 2021 •

edited

Loading

lepestock commented Mar 18, 2021

openjdk bot commented Mar 18, 2021

shipilev commented Apr 1, 2021

iignatev commented Apr 1, 2021

iignatev commented Apr 14, 2021

iignatev commented Apr 14, 2021

openjdk bot commented Apr 14, 2021

		@@ -63,10 +66,21 @@

		private static final boolean USE_THROW_CATCH = false; // Test bugs

		private static final MemoryPoolMXBean CODE_CACHE_MX_BEAN = ManagementFactory

8058176: [mlvm] tests should not allow code cache exhaustion #2523

8058176: [mlvm] tests should not allow code cache exhaustion #2523

Conversation

lepestock commented Feb 11, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 11, 2021

openjdk bot commented Feb 11, 2021

mlbridge bot commented Feb 11, 2021 • edited Loading

Webrevs

lepestock commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Mar 16, 2021 • edited Loading

lepestock commented Mar 18, 2021

openjdk bot commented Mar 18, 2021

shipilev commented Apr 1, 2021

iignatev commented Apr 1, 2021

iignatev commented Apr 14, 2021

iignatev commented Apr 14, 2021

openjdk bot commented Apr 14, 2021

lepestock commented Feb 11, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 11, 2021 •

edited

Loading

openjdk bot commented Mar 16, 2021 •

edited

Loading