8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout #21662

stefank · 2024-10-23T11:39:26Z

When testing Lilliput we found a failure in serviceability/sa/ClhsdbJstackWithConcurrentLock.java test when running with C1-only.

The test uses the SA's thread printing feature to print the threads and the "concurrent locks" / AbstractOwnableSynchronizers. It then verifies that the expected lock is listed in the section for "Locked ownable synchronizers".

When we turned on Lilliput's -XX:+UseCompactObjectHeaders this stopped working, and we got nothing reported in that section:

"Thread-0" #31 prio=5 tid=0x00007a708c259ad0 nid=1480533 waiting on condition [0x00007a706fefe000]
   java.lang.Thread.State: TIMED_WAITING (sleeping)
   JavaThread state: _thread_blocked
 - java.lang.Thread.sleepNanos0(long) @bci=0 (Interpreted frame)
 - java.lang.Thread.sleepNanos(long) @bci=33, line=497 (Interpreted frame)
 - java.lang.Thread.sleep(long) @bci=25, line=528 (Interpreted frame)
 - LingeredAppWithConcurrentLock.lockMethod(java.util.concurrent.locks.Lock) @bci=13, line=38 (Interpreted frame)
	- locked <0x00000000ffd32d88> (a java.util.concurrent.locks.ReentrantLock)
 - LingeredAppWithConcurrentLock.lambda$main$0() @bci=3, line=46 (Interpreted frame)
 - LingeredAppWithConcurrentLock$$Lambda+0x00007a7023001000.run() @bci=0 (Interpreted frame)
 - java.lang.Thread.runWith(java.lang.Object, java.lang.Runnable) @bci=5, line=1589 (Interpreted frame)
 - java.lang.Thread.run() @bci=19, line=1576 (Interpreted frame)

Locked ownable synchronizers:
    - None

It should be saying:

Locked ownable synchronizers:
    - <0x00000000ffd32d88>, (a java/util/concurrent/locks/ReentrantLock$NonfairSync)

The problem lies within the code that searches for objects in the heap. It collects a bunch of regions and searches them for objects. However, the code that describes the TLAB regions are stale and doesn't match the C++ implementation in the JVM. When Lilliput shrinks the headers the SA code is broken enough to cause the TLAB regions to be reported as overlapping. This has ripple effects that the object iterators stop working.

I can get this test to pass, with and without compact object headers, by fixing the code in ThreadLocalAllocBuffer::hard_end().

This is a reproducer of the problem:

make -C ../build/fastdebug test TEST=serviceability/sa/ClhsdbJstackWithConcurrentLock.java JTREG="JAVA_OPTIONS=-XX:TieredStopAtLevel=2 -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders"

I've tested this by running all 'serviceability' tests in our tier1-9 testing.

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-8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout (Bug - P4)

Reviewers

Roman Kennke (@rkennke - Reviewer) Review applies to 3f50944e
Leonid Mesnik (@lmesnik - Reviewer) Review applies to 3f50944e
Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21662

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-23T11:40:07Z

👋 Welcome back stefank! 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-10-23T11:40:46Z

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

8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout

Reviewed-by: cjplummer, rkennke, lmesnik

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

c548986: 8342918: Building ParallelGC without G1 and Shenandoah fails because SpaceMangler not declared
ca1700b: 8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer
d1540e2: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
7af46a6: 8340554: Improve MessageFormat readObject checks
7d5eefa: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
d8c3b0f: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
3c14c2b: 8341566: Add Reader.of(CharSequence)
b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
... and 16 more: https://git.openjdk.org/jdk/compare/7131f053b0d26b62cbf0d8376ec117d6e8d79f9e...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-10-23T11:41:26Z

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

hotspot-gc
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-10-23T11:44:37Z

Webrevs

rkennke

The changes make sense to me and the code now matches the C++ implementation on HotSpot. Thanks!

plummercj · 2024-10-23T19:48:13Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VM.java

       reserveForAllocationPrefetch = (int)reserveForAllocationPrefetchField.getCInteger(intType);
+
+       Type collectedHeap = db.lookupType("CollectedHeap");
+       CIntegerType sizeType = (CIntegerType) db.lookupType("size_t");


I think you can use getSizet() here.

getSizet() seems to be a function in Flags, so I don't see a direct way to use it. I could probably use the sizetType instead of db.lookupType("size_t"), however when I tested the tests failed because sizetType had not been initialized yet.

It looks like sizetType is initialized further down in the constructor. You could move that up (and the other 6 types also) or move your new code down. And speaking of the new code, it is misplaced in the try block. Both the comment for that block and the exception thrown have to do with getting the version. The reserveForAllocationPrefetch code just above is also misplaced for the same reason. That got added by JDK-8004710. Perhaps some cleanup is in order here. I don't think a try/catch/throw is needed for the tlab code. There are other places in the constructor that can throw an exception, and if that happens, it should be obvious from the stack trace where it happened. That is all you need to debug the issue. I think maybe the VMVersion section was special cased since it is the most likely failure point if something is really wrong that will result in a failure of SA to startup.

OK. I did some cleanups. Could you take a look and see if this is good enough to get this integrated?

This reverts commit e8ec295.

plummercj

Looks good. Please update the SA copyrights before integrating.

stefank · 2024-10-25T06:45:07Z

/integrate

openjdk · 2024-10-25T06:46:22Z

@stefank This pull request has not yet been marked as ready for integration.

stefank · 2024-10-25T07:25:16Z

Thanks for the reviews!
/integrate

openjdk · 2024-10-25T07:26:23Z

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

4635351: 8342939: Building ZGC without compiler2 fails
fd5ff05: 8342827: Fix order of @param tags in other modules
415d815: 8342938: Problem list java/io/IO/IO.java test on Linux ppc64le
c548986: 8342918: Building ParallelGC without G1 and Shenandoah fails because SpaceMangler not declared
ca1700b: 8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer
d1540e2: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
7af46a6: 8340554: Improve MessageFormat readObject checks
7d5eefa: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
d8c3b0f: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
3c14c2b: 8341566: Add Reader.of(CharSequence)
... and 19 more: https://git.openjdk.org/jdk/compare/7131f053b0d26b62cbf0d8376ec117d6e8d79f9e...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-25T07:26:36Z

@stefank Pushed as commit 3c5db12.

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

8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout

be690e2

stefank mentioned this pull request Oct 23, 2024

8305895: Implement JEP 450: Compact Object Headers (Experimental) #20677

Closed

21 tasks

openjdk bot added the rfr Pull request is ready for review label Oct 23, 2024

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot-gc hotspot-gc-dev@openjdk.org labels Oct 23, 2024

rkennke approved these changes Oct 23, 2024

View reviewed changes

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

lmesnik approved these changes Oct 23, 2024

View reviewed changes

plummercj suggested changes Oct 23, 2024

View reviewed changes

Remove redundant db.lookupType

e8ec295

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

Revert "Remove redundant db.lookupType"

3f50944

This reverts commit e8ec295.

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

Cleanups

adc7d75

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

plummercj approved these changes Oct 24, 2024

View reviewed changes

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

Copyright

458165b

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

plummercj approved these changes Oct 25, 2024

View reviewed changes

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

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

openjdk bot closed this Oct 25, 2024

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

8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout #21662

8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout #21662

Uh oh!

Conversation

stefank commented Oct 23, 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 Oct 23, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

rkennke left a comment

Choose a reason for hiding this comment

Uh oh!

plummercj Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

stefank Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

plummercj Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

stefank Oct 24, 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!

plummercj left a comment

Choose a reason for hiding this comment

Uh oh!

stefank commented Oct 25, 2024

Uh oh!

openjdk bot commented Oct 25, 2024

Uh oh!

stefank commented Oct 25, 2024

Uh oh!

openjdk bot commented Oct 25, 2024

Uh oh!

openjdk bot commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

stefank commented Oct 23, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 23, 2024 •

edited

Loading

mlbridge bot commented Oct 23, 2024 •

edited

Loading

stefank Oct 24, 2024 •

edited

Loading