8247281: migrate ObjectMonitor::_object to OopStorage #135

dcubed-ojdk · 2020-09-11T18:45:28Z

This RFE is to migrate the following field to OopStorage:

class ObjectMonitor {

void* volatile _object; // backward object pointer - strong root

Unlike the previous patches in this series, there are a lot of collateral
changes so this is not a trivial review. Sorry for the tedious parts of
the review. Since Erik and I are both contributors to this patch, we
would like at least 1 GC team reviewer and 1 Runtime team reviewer.

This changeset was tested with Mach5 Tier[1-3],4,5,6,7,8 testing
along with JDK-8252980 and JDK-8252981. I also ran it through my
inflation stress kit for 48 hours on my Linux-X64 machine.

Progress

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

Issue

JDK-8247281: migrate ObjectMonitor::_object to OopStorage

Reviewers

Erik Österlund (@fisk - Reviewer)
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to eeb9d76
David Holmes (@dholmes-ora - Reviewer)
Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to 750fe77
Kim Barrett (@kimbarrett - Reviewer)
Roman Kennke (@rkennke - Reviewer) ⚠️ Review applies to eeb9d76
Serguei Spitsyn (@sspitsyn - Reviewer)

Contributors

Erik Österlund <erik.osterlund@oracle.com>
Daniel Daugherty <daniel.daugherty@oracle.com>

Download

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

dcubed-ojdk · 2020-09-11T18:46:05Z

/contributor add Erik Österlund erik.osterlund@oracle.com
/contributor add Daniel Daugherty daniel.daugherty@oracle.com

bridgekeeper · 2020-09-11T18:47:21Z

👋 Welcome back dcubed! 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-09-11T18:47:49Z

@dcubed-ojdk Could not parse Erik Österlund erik.osterlund@oracle.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

openjdk · 2020-09-11T18:47:52Z

@dcubed-ojdk Could not parse Daniel Daugherty daniel.daugherty@oracle.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

openjdk · 2020-09-11T18:48:10Z

@dcubed-ojdk The following labels will be automatically applied to this pull request: 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 (add|remove) "label" command.

dcubed-ojdk · 2020-09-11T18:48:41Z

/contributor add Erik Österlund erik.osterlund@oracle.com
/contributor add Daniel Daugherty daniel.daugherty@oracle.com

dcubed-ojdk · 2020-09-11T18:49:29Z

/label remove serviceability,hotspot
/label add hotspot-runtime,hotspot-gc

openjdk · 2020-09-11T18:50:10Z

@dcubed-ojdk
The serviceability label was successfully removed.

The hotspot label was successfully removed.

openjdk · 2020-09-11T18:50:30Z

@dcubed-ojdk
Contributor Erik Österlund <erik.osterlund@oracle.com> successfully added.

openjdk · 2020-09-11T18:50:50Z

@dcubed-ojdk
Contributor Daniel Daugherty <daniel.daugherty@oracle.com> successfully added.

openjdk · 2020-09-11T18:51:18Z

@dcubed-ojdk
The hotspot-runtime label was successfully added.

The hotspot-gc label was successfully added.

dcubed-ojdk · 2020-09-11T18:52:07Z

@fisk - Please chime in on this review when you get the chance.

mlbridge · 2020-09-11T18:55:49Z

Webrevs

dcubed-ojdk

The vast majority of these changes came from @fisk so I'm actually a
reviewer and stress tester of these changes. Thumbs up!

fisk

This looks great Dan. I like your addition of is_chainmarker() - it makes this step a lot better, without having any special oops that are not oops. Thanks for sorting this out!

openjdk · 2020-09-11T19:19:26Z

@dcubed-ojdk This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8247281: migrate ObjectMonitor::_object to OopStorage

Co-authored-by: Erik Österlund <erik.osterlund@oracle.com>
Co-authored-by: Daniel Daugherty <daniel.daugherty@oracle.com>
Reviewed-by: eosterlund, coleenp, dholmes, stefank, kbarrett, rkennke, sspitsyn

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 104 commits pushed to the master branch:

f800af9: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
2e30ff6: 8253311: Cleanup relocInfo constructors
43be5a3: 8230652: Improve verbose output
5a7390b: 8253418: ZGC: Use pd_ prefix to denote platform dependent code
7e49eae: 8253407: Move max_pow2 from test to src
fbfb62d: 8253332: ZGC: Make heap views reservation platform independent
dad6edb: 8253321: java.util.Locale.LanguageRange#equals is inconsistent after calling hashCode
edc14f9: 8253286: Use expand_exact() instead of expand_at() for fixed requests in G1
bba948f: 8253411: [BACKOUT] [REDO] G1 incorrectly limiting young gen size when using the reserve can result in repeated full gcs
955c2e6: 8253303: G1: Move static initialization of G1FromCardCache to a proper location
... and 94 more: https://git.openjdk.java.net/jdk/compare/952518640a50ce86ca6f295867cfde93f6ac8b26...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate f800af978c42c88170def1b4996ffa524aaed4bd.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dcubed-ojdk · 2020-09-11T19:26:44Z

@fisk - Thanks for the blinding fast review! (Pretty easy when you wrote almost
all of the code).

Re: is_chainmarker()
I figured you would appreciate getting rid of one more "special" oop value!
And its use just fits in with the whole AllocationState model. It also gets
removed quite easily with the part3 patch... Oh yeah, I gotta file a new RFE
for that one.

coleenp · 2020-09-11T20:31:49Z

src/hotspot/share/prims/jvmtiTagMap.cpp

@@ -3017,13 +3017,6 @@ inline bool VM_HeapWalkOperation::collect_simple_roots() {
    return false;
  }

-  // Inflated monitors
-  blk.set_kind(JVMTI_HEAP_REFERENCE_MONITOR);


So we don't have to provide the equivalent of JVMTI_HEAP_REFERENCE_MONITOR?

The JVMTI roots are strong roots. This is no longer a strong root, so reporting them would be a bug after this change.

The function is VM_HeapWalkOperation::collect_simple_roots()
and we no longer have a root in the ObjectMonitor so my take is
no we don't. I believe @fisk concurs with that reasoning.

Yes, exactly.

Thanks for confirmation.

From the spec I'm not clear on exactly what JVMTI_HEAP_REFERENCE_MONITOR is intended to be. Serviceability folk should be giving some input here though.

I've taken a first pass at creating a CSR:
JDK-8253121 migrate ObjectMonitor::_object to OopStorage
https://bugs.openjdk.java.net/browse/JDK-8253121

Just a minor comment.
The line 1754 can be deleted as the JVMTI_HEAP_REFERENCE_MONITOR reference type will be never encountered:

1750 static jvmtiHeapRootKind toJvmtiHeapRootKind(jvmtiHeapReferenceKind kind) {
1751 switch (kind) {
1752 case JVMTI_HEAP_REFERENCE_JNI_GLOBAL: return JVMTI_HEAP_ROOT_JNI_GLOBAL;
1753 case JVMTI_HEAP_REFERENCE_SYSTEM_CLASS: return JVMTI_HEAP_ROOT_SYSTEM_CLASS;
1754 case JVMTI_HEAP_REFERENCE_MONITOR: return JVMTI_HEAP_ROOT_MONITOR;
1755 case JVMTI_HEAP_REFERENCE_STACK_LOCAL: return JVMTI_HEAP_ROOT_STACK_LOCAL;
1756 case JVMTI_HEAP_REFERENCE_JNI_LOCAL: return JVMTI_HEAP_ROOT_JNI_LOCAL;
1757 case JVMTI_HEAP_REFERENCE_THREAD: return JVMTI_HEAP_ROOT_THREAD;
1758 case JVMTI_HEAP_REFERENCE_OTHER: return JVMTI_HEAP_ROOT_OTHER;
1759 default: ShouldNotReachHere(); return JVMTI_HEAP_ROOT_OTHER;
1760 }
1761 }

Other than that the update in this file looks okay to me.

I cleaned that up. The only references to JVMTI_HEAP_REFERENCE_MONITOR and
JVMTI_HEAP_ROOT_MONITOR that remain are in the spec:

$ egrep -r 'JVMTI_HEAP_REFERENCE_MONITOR|JVMTI_HEAP_ROOT_MONITOR' src/hotspot
src/hotspot/share/prims/jvmti.xml:
src/hotspot/share/prims/jvmti.xml:
src/hotspot/share/prims/jvmti.xml: JVMTI_HEAP_ROOT_MONITOR,

src/hotspot/share/runtime/objectMonitor.cpp

coleenp · 2020-09-11T20:51:20Z

src/hotspot/share/services/heapDumper.cpp

-  }
-  void do_oop(oop* obj_p) {
-    u4 size = 1 + sizeof(address);
-    writer()->start_sub_record(HPROF_GC_ROOT_MONITOR_USED, size);


I had a similar question to the jvmtiTagMap question above. Are there tools that are going to miss seeing this tag in the heap dump? I hope these tags are implementation defined and we can just remove them. Otherwise, should there be a loop through the OM list to print out these tags for live object monitors?

Same answer as above. No longer a strong root, so can't report it as such.

We no longer have a root in the ObjectMonitor so no we don't have
to dump these is my take. I believe @fisk concurs with that reasoning.

Ok, that's good then!

Thanks for confirmation.

I don't see anything in the HPROF format description that claims this is a strong root. At a minimum this seems to be a behavioural change that would warrant a CSR request. This also seems to be something that the serviceability folk should be made aware of and have a chance to comment on.

I've taken a first pass at creating a CSR:
JDK-8253121 migrate ObjectMonitor::_object to OopStorage
https://bugs.openjdk.java.net/browse/JDK-8253121

I've looked at the CSR and added myself as a reviewer.
We had a slack chat with Dan, and I agree that with a weak handle it would be racy/unsafe for JVMTI_HEAP_REFERENCE_MONITOR calls back to be called. The ObjectMonitors do not pin objects anymore (there are no strong refs from them), so it has to be okay to skip reporting the JVMTI_HEAP_REFERENCE_MONITOR and and JVMTI_HEAP_ROOT_MONITOR (old Heap Walking API) reference types. The JVMTI does not need an update as other VM implementations can still report these reference types. Alan added a comment to the CSR saying that memory profiling tools that use the JVMTI functions (FollowReferences with jvmtiHeapReferenceCallback or IterateOverReachableObjects with jvmtiHeapRootCallback) to iterate over the heap should not have a compatibility impact as these reference types are just informational but adding a release note can be useful.

Slight clarification. Serguei and I were discussing whether we could continue
to make JVMTI_HEAP_REFERENCE_MONITOR call backs or emit
HPROF_GC_ROOT_MONITOR_USED entries in heap dump output as a
way to ease the transition phase of getting used to these things going
away. My answer was that we could do that but it would racy and unsafe
due to the ObjectMonitor's object being GC'ed.

It's also incorrect to make JVMTI_HEAP_REFERENCE_MONITOR call backs
or emit HPROF_GC_ROOT_MONITOR_USED entries in heap dump once
the ObjectMonitor's object ref becomes a weak handle. That weak handle
no longer prevents the associated object from being GC'ed so it is no
longer a strong root.

See Erik's comment above: #135 (comment)

src/hotspot/share/runtime/synchronizer.cpp

dcubed-ojdk · 2020-09-14T18:59:06Z

@coleenp, @rkennke, and @kimbarrett - I believe all of the changes that you have requested
have been made. Please confirm by re-reviewing.

@dholmes-ora - I don't think you asked for any specific code changes. I've taken a first pass
at creating a CSR:
JDK-8253121 migrate ObjectMonitor::_object to OopStorage
https://bugs.openjdk.java.net/browse/JDK-8253121

Please look it over and feel free to edit as needed. Since I don't do
CSR's often, what I've done might be all wrong. :-)

rkennke · 2020-09-14T20:05:06Z

@coleenp, @rkennke, and @kimbarrett - I believe all of the changes that you have requested
have been made. Please confirm by re-reviewing.

It looks good to me now! I've also build with Shenandoah GC, and run our sanity-tests (TEST=hotspot_gc_shenandoah) and all looks good. Thank you!

src/hotspot/share/gc/shared/space.inline.hpp

dcubed-ojdk · 2020-09-14T20:22:10Z

@rkennke - Thanks for confirming that Shenandoah now builds with the changes
that you provided. Thanks for sending those.

…h about markWords.

dholmes-ora · 2020-09-15T01:55:53Z

/csr needed

Edit: I missed that this was already flagged.

openjdk · 2020-09-15T01:57:16Z

@dholmes-ora an approved CSR request is already required for this pull request.

kimbarrett · 2020-09-15T10:17:45Z

src/hotspot/share/runtime/objectMonitor.cpp

@@ -241,13 +241,12 @@ void ObjectMonitor::operator delete[] (void *p) {
  operator delete(p);
 }

+#ifdef ASSERT


There would be less #ifdef ASSERT clutter if just the body of check_object_context were conditionalized. Then the calls wouldn't need to be. Your call...

I'll make that change.

coleenp

Thank you for removing it.

rkennke

Looks good to me!

sspitsyn · 2020-09-16T00:20:16Z

src/hotspot/share/prims/jvmtiTagMap.cpp

@@ -3017,13 +3017,6 @@ inline bool VM_HeapWalkOperation::collect_simple_roots() {
    return false;
  }

-  // Inflated monitors
-  blk.set_kind(JVMTI_HEAP_REFERENCE_MONITOR);


Just a minor comment.
The line 1754 can be deleted as the JVMTI_HEAP_REFERENCE_MONITOR reference type will be never encountered:

1750 static jvmtiHeapRootKind toJvmtiHeapRootKind(jvmtiHeapReferenceKind kind) {
1751 switch (kind) {
1752 case JVMTI_HEAP_REFERENCE_JNI_GLOBAL: return JVMTI_HEAP_ROOT_JNI_GLOBAL;
1753 case JVMTI_HEAP_REFERENCE_SYSTEM_CLASS: return JVMTI_HEAP_ROOT_SYSTEM_CLASS;
1754 case JVMTI_HEAP_REFERENCE_MONITOR: return JVMTI_HEAP_ROOT_MONITOR;
1755 case JVMTI_HEAP_REFERENCE_STACK_LOCAL: return JVMTI_HEAP_ROOT_STACK_LOCAL;
1756 case JVMTI_HEAP_REFERENCE_JNI_LOCAL: return JVMTI_HEAP_ROOT_JNI_LOCAL;
1757 case JVMTI_HEAP_REFERENCE_THREAD: return JVMTI_HEAP_ROOT_THREAD;
1758 case JVMTI_HEAP_REFERENCE_OTHER: return JVMTI_HEAP_ROOT_OTHER;
1759 default: ShouldNotReachHere(); return JVMTI_HEAP_ROOT_OTHER;
1760 }
1761 }

Other than that the update in this file looks okay to me.

fisk · 2020-09-16T08:23:03Z

I added a release note (https://bugs.openjdk.java.net/browse/JDK-8253225) describing that these roots are now weak, and hence won't be reported. Please have a look at that, to make sure what I am describing makes sense.

dcubed-ojdk · 2020-09-16T15:59:19Z

@kimbarrett - Thanks for the cleanup suggestion.
@sspitsyn - Thanks for the review.
I've gone ahead and "resolved" the comments that were addressed by previous commits.
I've made changes for both Kim and Serguei's reviews. Building those bits now.

dcubed-ojdk · 2020-09-16T16:08:44Z

@kimbarrett and @sspitsyn - Your most recent comments should be resolved via 215084a.

dcubed-ojdk · 2020-09-21T22:11:13Z

/integrate

openjdk · 2020-09-21T22:12:25Z

@dcubed-ojdk Since your change was applied there have been 104 commits pushed to the master branch:

f800af9: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()
2e30ff6: 8253311: Cleanup relocInfo constructors
43be5a3: 8230652: Improve verbose output
5a7390b: 8253418: ZGC: Use pd_ prefix to denote platform dependent code
7e49eae: 8253407: Move max_pow2 from test to src
fbfb62d: 8253332: ZGC: Make heap views reservation platform independent
dad6edb: 8253321: java.util.Locale.LanguageRange#equals is inconsistent after calling hashCode
edc14f9: 8253286: Use expand_exact() instead of expand_at() for fixed requests in G1
bba948f: 8253411: [BACKOUT] [REDO] G1 incorrectly limiting young gen size when using the reserve can result in repeated full gcs
955c2e6: 8253303: G1: Move static initialization of G1FromCardCache to a proper location
... and 94 more: https://git.openjdk.java.net/jdk/compare/952518640a50ce86ca6f295867cfde93f6ac8b26...master

Your commit was automatically rebased without conflicts.

Pushed as commit d8921ed.

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

8247281: migrate ObjectMonitor::_object to OopStorage

150a398

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Sep 11, 2020

openjdk bot removed serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 11, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 11, 2020

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Sep 11, 2020

dcubed-ojdk marked this pull request as ready for review September 11, 2020 18:52

openjdk bot added the rfr Pull request is ready for review label Sep 11, 2020

dcubed-ojdk commented Sep 11, 2020

View reviewed changes

fisk approved these changes Sep 11, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 11, 2020

coleenp suggested changes Sep 11, 2020

View reviewed changes

coleenp CR - changes to resolve Coleen's comments.

750fe77

rkennke reviewed Sep 14, 2020

View reviewed changes

src/hotspot/share/gc/shared/space.inline.hpp Outdated Show resolved Hide resolved

rkennke, coleenp, fisk CR - delete random assert() that knows too muc…

eeb9d76

…h about markWords.

dholmes-ora approved these changes Sep 15, 2020

View reviewed changes

kimbarrett reviewed Sep 15, 2020

View reviewed changes

kimbarrett approved these changes Sep 15, 2020

View reviewed changes

coleenp approved these changes Sep 15, 2020

View reviewed changes

rkennke approved these changes Sep 15, 2020

View reviewed changes

sspitsyn approved these changes Sep 16, 2020

View reviewed changes

Minor changes/deletions to address kimbarrett and sspitsyn CR comments.

215084a

sspitsyn approved these changes Sep 16, 2020

View reviewed changes

fisk approved these changes Sep 16, 2020

View reviewed changes

dholmes-ora approved these changes Sep 17, 2020

View reviewed changes

kimbarrett approved these changes Sep 17, 2020

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 21, 2020

openjdk bot closed this Sep 21, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Sep 21, 2020

openjdk bot removed the rfr Pull request is ready for review label Sep 21, 2020

dcubed-ojdk deleted the JDK-8247281 branch September 22, 2020 16:12

robehn pushed a commit to robehn/jdk that referenced this pull request Nov 13, 2023

Merge jdk-22+15 into rivos/main (openjdk#135)

d8b9bdd

8247281: migrate ObjectMonitor::_object to OopStorage #135

8247281: migrate ObjectMonitor::_object to OopStorage #135

Conversation

dcubed-ojdk commented Sep 11, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Download

dcubed-ojdk commented Sep 11, 2020

bridgekeeper bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

dcubed-ojdk commented Sep 11, 2020 • edited Loading

dcubed-ojdk commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

openjdk bot commented Sep 11, 2020

dcubed-ojdk commented Sep 11, 2020

mlbridge bot commented Sep 11, 2020 • edited Loading

Webrevs

dcubed-ojdk left a comment

Choose a reason for hiding this comment

fisk left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 11, 2020 • edited Loading

dcubed-ojdk commented Sep 11, 2020

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

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

dcubed-ojdk commented Sep 14, 2020

rkennke commented Sep 14, 2020

dcubed-ojdk commented Sep 14, 2020

dholmes-ora commented Sep 15, 2020 • edited Loading

openjdk bot commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

rkennke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisk commented Sep 16, 2020

dcubed-ojdk commented Sep 16, 2020

dcubed-ojdk commented Sep 16, 2020

dcubed-ojdk commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

dcubed-ojdk commented Sep 11, 2020 •

edited by openjdk bot

Loading

dcubed-ojdk commented Sep 11, 2020 •

edited

Loading

mlbridge bot commented Sep 11, 2020 •

edited

Loading

openjdk bot commented Sep 11, 2020 •

edited

Loading

dholmes-ora commented Sep 15, 2020 •

edited

Loading