8212879: Make JVMTI TagMap table concurrent #967

coleenp · 2020-10-30T20:23:04Z

This change turns the HashTable that JVMTI uses for object tagging into a regular Hotspot hashtable - the one in hashtable.hpp with resizing and rehashing. Instead of pointing directly to oops so that GC has to walk the table to follow oops and then to rehash the table, this table points to WeakHandle. GC walks the backing OopStorages concurrently.

The hash function for the table is a hash of the lower 32 bits of the address. A flag is set during GC (gc_notification if in a safepoint, and through a call to JvmtiTagMap::needs_processing()) so that the table is rehashed at the next use.

The gc_notification mechanism of weak oop processing is used to notify Jvmti to post ObjectFree events. In concurrent GCs there can be a window of time between weak oop marking where the oop is unmarked, so dead (the phantom load in peek returns NULL) but the gc_notification hasn't been done yet. In this window, a heap walk or GetObjectsWithTags call would not find an object before the ObjectFree event is posted. This is dealt with in two ways:

In the Heap walk, there's an unconditional table walk to post events if events are needed to post.
For GetObjectWithTags, if a dead oop is found in the table and posting is required, we use the VM thread to post the event.

Event posting cannot be done in a JavaThread because the posting needs to be done while holding the table lock, so that the JvmtiEnv state doesn't change before posting is done. ObjectFree callbacks are limited in what they can do as per the JVMTI Specification. The allowed callbacks to the VM already have code to allow NonJava threads.

To avoid rehashing, I also tried to use object->identity_hash() but this breaks because entries can be added to the table during heapwalk, where the objects use marking. The starting markWord is saved and restored. Adding a hashcode during this operation makes restoring the former markWord (locked, inflated, etc) too complicated. Plus we don't want all these objects to have hashcodes because locking operations after tagging would have to always use inflated locks.

Much of this change is to remove serial weak oop processing for the weakProcessor, ZGC and Shenandoah. The GCs have been stress tested with jvmti code.

It has also been tested with tier1-6.

Thank you to Stefan, Erik and Kim for their help with this change.

Progress

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

Testing

	Linux aarch64	Linux arm	Linux ppc64le	Linux s390x	Linux x64	Linux x86	Windows x64	macOS x64
Build	⏳ (1/1 running)	⏳ (1/1 running)	⏳ (1/1 running)	⏳ (1/1 running)	✔️ (6/6 passed)	✔️ (2/2 passed)	⏳ (1/2 running)	✔️ (2/2 passed)
Test (tier1)					⏳ (9/9 running)	⏳ (4/9 running)		⏳ (9/9 running)

Issue

JDK-8212879: Make JVMTI TagMap table concurrent

Reviewers

Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to cb4c83e
Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 534326d
Zhengyu Gu (@zhengyu123 - Reviewer) ⚠️ Review applies to cb4c83e
Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to eeaf9ae
Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 1940eaf
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 1940eaf

Contributors

Kim Barrett <kbarrett@openjdk.org>
Coleen Phillimore <coleenp@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/967/head:pull/967
$ git checkout pull/967

bridgekeeper · 2020-10-30T20:24:48Z

👋 Welcome back coleenp! 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 · 2020-10-30T20:26:48Z

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

build
hotspot
serviceability
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.

mlbridge · 2020-10-30T20:31:08Z

Webrevs

erikj79

Build changes look ok.

stefank

Commented on nits, and reviewed GC code and tag map code. Didn't look closely on the hashmap changes.

src/hotspot/share/gc/shared/oopStorageSet.hpp

src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp

src/hotspot/share/gc/z/zOopClosures.hpp

src/hotspot/share/prims/jvmtiExport.hpp

src/hotspot/share/prims/jvmtiTagMap.cpp

src/hotspot/share/prims/jvmtiTagMapTable.cpp

test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp

magicus

Build changes look good. Not reviewed hotspot changes.

openjdk · 2020-11-02T12:29:20Z

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

8212879: Make JVMTI TagMap table concurrent

Co-authored-by: Kim Barrett <kbarrett@openjdk.org>
Co-authored-by: Coleen Phillimore <coleenp@openjdk.org>
Reviewed-by: stefank, ihse, zgu, eosterlund, sspitsyn, kbarrett

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

3a4b90f: 8202343: Disable TLS 1.0 and 1.1
342ccf6: 8256253: Defer Biased Locking obsoletion to JDK 18
d183fc7: 8221554: aarch64 cross-modifying code
f626ed6: 8255978: [windows] os::release_memory may not release the full range
6702910: 8256375: AArch64: aarch64-asmtest.py may generate undefined register r18
9fe2d31: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest
cb2676c: 8256499: Zero: enable Epsilon GC
8e241b5: 8256552: Let ReplayCompiles set UseDebuggerErgo
4178834: 8256172: Clean up CDS handling of i2i_entry
cfa92a5: 8256178: Add RAII object for file lock

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

coleenp

I think I addressed your comments, retesting now. Thank you!

src/hotspot/share/gc/shared/oopStorageSet.hpp

src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp

src/hotspot/share/gc/z/zOopClosures.hpp

src/hotspot/share/prims/jvmtiExport.hpp

src/hotspot/share/prims/jvmtiTagMap.cpp

src/hotspot/share/prims/jvmtiTagMapTable.cpp

test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp

coleenp · 2020-11-02T13:25:53Z

There should be a thank you emoji that doesn't send email except maybe to the person reviewing the code. Thank you @erikj79 and @magicus for reviewing the build changes. There should also be a 'fixed' emoji.

openjdk · 2020-11-02T15:55:11Z

@coleenp this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout jvmti-table
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

zhengyu123

Shenandoah part looks good.

fisk

Looks great in general. Great work Coleen, and thanks again for fixing this. I like all the red lines in the GC code. I added a few nits/questions.

test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/cm02t001.cpp

src/hotspot/share/prims/jvmtiTagMap.cpp

stefank

Some more nit-picking to make the code more consistent.

src/hotspot/share/prims/jvmtiTagMapTable.cpp

src/hotspot/share/prims/jvmtiTagMapTable.hpp

src/hotspot/share/prims/jvmtiTagMapTable.cpp

coleenp · 2020-11-16T23:13:09Z

I've added two commits from @kimbarrett that defer the ObjectFree posting to the service thread or to a place where it could be removed before posting.
I also remerged and added the call JvmtiTagMap::set_needs_cleaning() to shenandoah which works after merging the latest code from shenandoah.
Testing tiers 1-6 currently. jvmti/jdi tests pass with G1 and ZGC stress options, and JVMTI tests pass with shenandoah.
Thanks! Coleen

openjdk · 2020-11-16T23:14:06Z

@coleenp Unknown command author - for a list of valid commands use /help.

coleenp · 2020-11-16T23:32:37Z

/contributor add @kimbarrett
/contributor add @coleenp

openjdk · 2020-11-16T23:32:56Z

@coleenp
Contributor Kim Barrett <kbarrett@openjdk.org> successfully added.

openjdk · 2020-11-16T23:33:37Z

@coleenp
Contributor Coleen Phillimore <coleenp@openjdk.org> successfully added.

sspitsyn

Hi Coleen,
It looks good to me.
Just a couple of nits below.

src/hotspot/share/prims/jvmtiTagMap.cpp:

There is a double-check for _needs_cleaning, so the one at line 136 can be removed:

 136   if (_needs_cleaning &&
 137       post_events &&
 138       env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
 139     remove_dead_entries(true /* post_object_free */);

1158 void JvmtiTagMap::remove_dead_entries(bool post_object_free) {
1159   assert(is_locked(), "precondition");
1160   if (_needs_cleaning) {
1161     log_info(jvmti, table)("TagMap table needs cleaning%s",
1162                            (post_object_free ? " and posting" : ""));
1163     hashmap()->remove_dead_entries(env(), post_object_free);
1164     _needs_cleaning = false;
1165   }
1166 }

test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach021/attach021Agent00.cpp:

The change below is not needed as the call to nsk_jvmti_aod_disableEventAndFinish() does exactly the same:

-    nsk_jvmti_aod_disableEventAndFinish(agentName, JVMTI_EVENT_OBJECT_FREE, success, jvmti, jni);
+
+    /* Flush any pending ObjectFree events, which may set success to 1 */
+    if (jvmti->SetEventNotificationMode(JVMTI_DISABLE,
+                                        JVMTI_EVENT_OBJECT_FREE,
+                                        NULL) != JVMTI_ERROR_NONE) {
+        success = 0;
+    }
+
+    nsk_aod_agentFinished(jni, agentName, success);
 }

coleenp · 2020-11-18T23:21:09Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach021/attach021Agent00.cpp

@@ -69,7 +69,15 @@ Java_nsk_jvmti_AttachOnDemand_attach021_attach021Target_setTagFor(JNIEnv * jni,
 JNIEXPORT void JNICALL
 Java_nsk_jvmti_AttachOnDemand_attach021_attach021Target_shutdownAgent(JNIEnv * jni,
        jclass klass) {
-    nsk_jvmti_aod_disableEventAndFinish(agentName, JVMTI_EVENT_OBJECT_FREE, success, jvmti, jni);


@kimbarrett ? I'm not sure why you made this change. See Serguei's comment. It does look the same.

coleenp · 2020-11-18T23:26:05Z

There is a double-check for _needs_cleaning, so the one at line 136 can be removed:

I want to leave _needs_cleaning at 136 because even though the boolean is checked twice, it doesn't hurt performance and it has a nice symmetry in that function.

I asked Kim about the other change.

Thank you for reviewing, Serguei!

kimbarrett · 2020-11-19T00:39:52Z

test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach021/attach021Agent00.cpp:

The change below is not needed as the call to nsk_jvmti_aod_disableEventAndFinish() does exactly the same:

-    nsk_jvmti_aod_disableEventAndFinish(agentName, JVMTI_EVENT_OBJECT_FREE, success, jvmti, jni);
+
+    /* Flush any pending ObjectFree events, which may set success to 1 */
+    if (jvmti->SetEventNotificationMode(JVMTI_DISABLE,
+                                        JVMTI_EVENT_OBJECT_FREE,
+                                        NULL) != JVMTI_ERROR_NONE) {
+        success = 0;
+    }
+
+    nsk_aod_agentFinished(jni, agentName, success);
 }

This change really is needed.

The success variable in the test is a global, initially 0, set to 1 by the
ObjectFree handler.

In the old code, if the ObjectFree event hasn't been posted yet, we pass the
initial 0 value of success to nsk_jvmti_aod_disabledEventAndFinish, where
it's a local variable (so unaffected by any changes to the variable in the
test), so stays 0 through to the call to nsk_aod_agentFinished. So the test
fails.

The split in the change causes the updated post-ObjectFree event success
value of 1 to be passed to agentFinished. So the test passes.

That required some head scratching to find at the time. That's the point of
the comment about flushing pending events. Maybe the comment should be
improved.

sspitsyn · 2020-11-19T10:10:06Z

@kimbarrett
Okay, thank you for explanation.
I agree, it'd make sense to improve the comment a little bit.
Thanks,
Serguei

coleenp · 2020-11-19T12:36:46Z

So should nsk_jvmti_aod_disableEventAndFinish pass the address of success instead? Why didn't it fail before this change?
Ok, with this change, it's not posted yet and the success variable for nsk_aod_agentFinished is the local variable. We should fix this in an RFE filed: https://bugs.openjdk.java.net/browse/JDK-8256651

coleenp · 2020-11-19T12:51:11Z

/* Flush any pending ObjectFree events, which will set global success variable to 1
   for any pending ObjectFree events. */

How about this? The word 'global' helps me.

coleenp · 2020-11-19T12:54:23Z

With remerging into shenandoah, all the jdi tests pass with shenandoah also.

coleenp · 2020-11-19T14:29:28Z

Thank you to all the reviewers.
/integrate

openjdk · 2020-11-19T14:30:31Z

@coleenp Since your change was applied there have been 10 commits pushed to the master branch:

3a4b90f: 8202343: Disable TLS 1.0 and 1.1
342ccf6: 8256253: Defer Biased Locking obsoletion to JDK 18
d183fc7: 8221554: aarch64 cross-modifying code
f626ed6: 8255978: [windows] os::release_memory may not release the full range
6702910: 8256375: AArch64: aarch64-asmtest.py may generate undefined register r18
9fe2d31: 8252304: Seed an HttpRequest.Builder from an existing HttpRequest
cb2676c: 8256499: Zero: enable Epsilon GC
8e241b5: 8256552: Let ReplayCompiles set UseDebuggerErgo
4178834: 8256172: Clean up CDS handling of i2i_entry
cfa92a5: 8256178: Add RAII object for file lock

Your commit was automatically rebased without conflicts.

Pushed as commit ba721f5.

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

Co-authored-by: Kim Barrett <kbarrett@openjdk.org> Co-authored-by: Coleen Phillimore <coleenp@openjdk.org> Reviewed-by: stefank, ihse, zgu, eosterlund, sspitsyn, kbarrett

8212879: Make JVMTI TagMap table not hash on oop address

534326d

openjdk bot added the rfr Pull request is ready for review label Oct 30, 2020

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Oct 30, 2020

erikj79 reviewed Oct 30, 2020

View reviewed changes

stefank reviewed Nov 2, 2020

View reviewed changes

magicus approved these changes Nov 2, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 2, 2020

coleenp commented Nov 2, 2020

View reviewed changes

coleenp changed the title ~~8212879: Make JVMTI TagMap table not hash on oop address~~ 8212879: Make JVMTI TagMap table concurrent Nov 2, 2020

Code review comments from StefanK.

cb4c83e

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Nov 2, 2020

zhengyu123 approved these changes Nov 2, 2020

View reviewed changes

fisk suggested changes Nov 2, 2020

View reviewed changes

stefank approved these changes Nov 3, 2020

View reviewed changes

More review comments from Stefan and ErikO

eeaf9ae

fisk approved these changes Nov 3, 2020

View reviewed changes

stefank added 2 commits November 3, 2020 14:22

Merge branch 'master' into jvmti-table

9eb335e

Merge branch 'master' into jvmti-table

1c3f2e1

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Nov 3, 2020

Fix minimal build.

1940eaf

kimbarrett approved these changes Nov 17, 2020

View reviewed changes

sspitsyn approved these changes Nov 18, 2020

View reviewed changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Nov 18, 2020

coleenp commented Nov 18, 2020

View reviewed changes

Merge branch 'master' into jvmti-table

9ef44f2

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Nov 19, 2020

Update comment in jvmti test.

40168e6

Fix copyrights in test changes.

589e4c5

openjdk bot closed this Nov 19, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 19, 2020

openjdk-notifier bot referenced this pull request Nov 19, 2020

8212879: Make JVMTI TagMap table concurrent

ba721f5

Co-authored-by: Kim Barrett <kbarrett@openjdk.org> Co-authored-by: Coleen Phillimore <coleenp@openjdk.org> Reviewed-by: stefank, ihse, zgu, eosterlund, sspitsyn, kbarrett

coleenp deleted the jvmti-table branch November 19, 2020 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8212879: Make JVMTI TagMap table concurrent #967

8212879: Make JVMTI TagMap table concurrent #967

coleenp commented Oct 30, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 30, 2020

openjdk bot commented Oct 30, 2020

mlbridge bot commented Oct 30, 2020 •

edited

Loading

erikj79 left a comment

stefank left a comment

magicus left a comment

openjdk bot commented Nov 2, 2020 •

edited

Loading

coleenp left a comment

coleenp commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

zhengyu123 left a comment

fisk left a comment

stefank left a comment

coleenp commented Nov 16, 2020 •

edited

Loading

openjdk bot commented Nov 16, 2020

coleenp commented Nov 16, 2020

openjdk bot commented Nov 16, 2020

openjdk bot commented Nov 16, 2020

sspitsyn left a comment

coleenp Nov 18, 2020 •

edited

Loading

coleenp commented Nov 18, 2020

kimbarrett commented Nov 19, 2020

sspitsyn commented Nov 19, 2020

coleenp commented Nov 19, 2020 •

edited

Loading

coleenp commented Nov 19, 2020

coleenp commented Nov 19, 2020

coleenp commented Nov 19, 2020

openjdk bot commented Nov 19, 2020

8212879: Make JVMTI TagMap table concurrent #967

8212879: Make JVMTI TagMap table concurrent #967

Conversation

coleenp commented Oct 30, 2020 • edited by openjdk bot Loading

Progress

Testing

Issue

Reviewers

Contributors

Download

bridgekeeper bot commented Oct 30, 2020

openjdk bot commented Oct 30, 2020

mlbridge bot commented Oct 30, 2020 • edited Loading

Webrevs

erikj79 left a comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

magicus left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 2, 2020 • edited Loading

coleenp left a comment

Choose a reason for hiding this comment

coleenp commented Nov 2, 2020

openjdk bot commented Nov 2, 2020

zhengyu123 left a comment

Choose a reason for hiding this comment

fisk left a comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

coleenp commented Nov 16, 2020 • edited Loading

openjdk bot commented Nov 16, 2020

coleenp commented Nov 16, 2020

openjdk bot commented Nov 16, 2020

openjdk bot commented Nov 16, 2020

sspitsyn left a comment

Choose a reason for hiding this comment

coleenp Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

coleenp commented Nov 18, 2020

kimbarrett commented Nov 19, 2020

sspitsyn commented Nov 19, 2020

coleenp commented Nov 19, 2020 • edited Loading

coleenp commented Nov 19, 2020

coleenp commented Nov 19, 2020

coleenp commented Nov 19, 2020

openjdk bot commented Nov 19, 2020

coleenp commented Oct 30, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 30, 2020 •

edited

Loading

openjdk bot commented Nov 2, 2020 •

edited

Loading

coleenp commented Nov 16, 2020 •

edited

Loading

coleenp Nov 18, 2020 •

edited

Loading

coleenp commented Nov 19, 2020 •

edited

Loading