jdk
jdk copied to clipboard
openjdk
8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout
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
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] 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)
- Leonid Mesnik (@lmesnik - 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
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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:
- c5489866308bd09729c5431537e0836145452624: 8342918: Building ParallelGC without G1 and Shenandoah fails because SpaceMangler not declared
- ca1700b8bf3b7b34616c469837748ac4c3607dfc: 8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer
- d1540e2a49c7a41eb771fc9896c367187d070dec: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
- 7af46a6b424cadfe298958d774da0f21db58ecd3: 8340554: Improve MessageFormat readObject checks
- 7d5eefa50673d6f7c5bd916f63271cf7898d6dee: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
- d8c3b0f834c603fe115ef4ca442727948b7a834e: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
- 3c14c2babbdfb46a77636ed80e083ef2f8be2b45: 8341566: Add Reader.of(CharSequence)
- b0ac633b2d0076d64b463b2a6ce19abf6b12c50f: 8342075: HttpClient: improve HTTP/2 flow control checks
- 85774b713edf8782f162ac25b61ce99a77e116f4: 8342882: RISC-V: Unify handling of jumps to runtime
- 2c31c8eeb42188ad6fd15eca50db4342cd791fb2: 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.
@stefank The following labels will be automatically applied to this pull request:
hotspot-gcserviceability
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.
Webrevs
- 04: Full - Incremental (458165b6)
- 03: Full - Incremental (adc7d755)
- 02: Full - Incremental (3f50944e)
- 01: Full - Incremental (e8ec2957)
- 00: Full (be690e24)
@stefank This pull request has not yet been marked as ready for integration.
Going to push as commit 3c5db12bbe4d1155ab874c2862005621c6b8541d.
Since your change was applied there have been 29 commits pushed to the master branch:
- 4635351b1570fcea07fac1ece5f76f528d68c2a7: 8342939: Building ZGC without compiler2 fails
- fd5ff0547ced6733ae05f1428664062615408dc9: 8342827: Fix order of @param tags in other modules
- 415d8151a6c7ce04d8ab27ff641acb5d69840125: 8342938: Problem list java/io/IO/IO.java test on Linux ppc64le
- c5489866308bd09729c5431537e0836145452624: 8342918: Building ParallelGC without G1 and Shenandoah fails because SpaceMangler not declared
- ca1700b8bf3b7b34616c469837748ac4c3607dfc: 8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer
- d1540e2a49c7a41eb771fc9896c367187d070dec: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
- 7af46a6b424cadfe298958d774da0f21db58ecd3: 8340554: Improve MessageFormat readObject checks
- 7d5eefa50673d6f7c5bd916f63271cf7898d6dee: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
- d8c3b0f834c603fe115ef4ca442727948b7a834e: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
- 3c14c2babbdfb46a77636ed80e083ef2f8be2b45: 8341566: Add Reader.of(CharSequence)
- ... and 19 more: https://git.openjdk.org/jdk/compare/7131f053b0d26b62cbf0d8376ec117d6e8d79f9e...master
Your commit was automatically rebased without conflicts.
@stefank Pushed as commit 3c5db12bbe4d1155ab874c2862005621c6b8541d.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.