8261675: ObjectValue::set_visited(bool) sets _visited false

[navyxliu]

The setter is error-prone. it unconditionally sets _visited false.
this patch stores the argument to it.

---

Progress

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

Issue

• JDK-8261675: ObjectValue::set_visited(bool) sets _visited false

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)

Download

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

[bridgekeeper]

👋 Welcome back xliu! 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]

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

• hotspot-compiler

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]

Webrevs

• 01: Full - Incremental (dce5bbf)
• 00: Full (2d512ad)

[vnkozlov]

Wow. This was original changes for first implementation of Escape Analysis 6558600.

But it works because the only place where set_visited() is called it sets to false:
debugInfoRec.cpp#L358

is_visited() is not called at all - _visited field is accessed directly only in one place:
debugInfo.cpp#L161

Would be nice to clean up this mess.

[vnkozlov]

In addition to your fix you can consider next changes to avoid dumping unneeded data in debug info (deoptimizer reads objects data only from top frame deoptimization.cpp#L190:

src/hotspot/share/opto/output.cpp
     // We dump the object pool first, since deoptimization reads it in first.
-    C->debug_info()->dump_object_pool(objs);
+    C->debug_info()->dump_object_pool(objs, (depth < max_depth));

src/hotspot/share/code/debugInfoRec.hpp
-  void dump_object_pool(GrowableArray<ScopeValue*>* objects);
+  void dump_object_pool(GrowableArray<ScopeValue*>* objects, bool visited = false);

src/hotspot/share/code/debugInfoRec.cpp
-void DebugInformationRecorder::dump_object_pool(GrowableArray<ScopeValue*>* objects) {
+void DebugInformationRecorder::dump_object_pool(GrowableArray<ScopeValue*>* objects, bool visited) {
   guarantee( _pcs_length > 0, "safepoint must exist before describing scopes");
   PcDesc* last_pd = &_pcs[_pcs_length-1];
   if (objects != NULL) {
     for (int i = objects->length() - 1; i >= 0; i--) {
-      objects->at(i)->as_ObjectValue()->set_visited(false);
+      objects->at(i)->as_ObjectValue()->set_visited(visited);
     }
   }

[navyxliu]

In addition to your fix you can consider next changes to avoid dumping unneeded data in debug info (deoptimizer reads objects data only from top frame deoptimization.cpp#L190:

src/hotspot/share/opto/output.cpp
     // We dump the object pool first, since deoptimization reads it in first.
-    C->debug_info()->dump_object_pool(objs);
+    C->debug_info()->dump_object_pool(objs, (depth < max_depth));

hi, Vladimir,

Thank you for reviewing this patch.

I don't understand why it's depth < max_depth instead of <=.
Further, the effect of this optimization seems limited. this statement is in a loop like this.
depth < max_depth is almost always true.

  for (int depth = 1; depth <= max_depth; depth++) {
...
    C->debug_info()->dump_object_pool(objs, (depth < max_depth));
...
  }

but I get your point. It seems that Process_OopMap_Node may dump identical objects in the loop. I am not sure there are objects overlap different jvmstates. let me check and create another issue if so.

[vnkozlov]

I am fine with your current change (getter and setter). Let push it.
And I will work on suggested optimization/clean in separate RFE (change will need comments to avoid confusion, as you have).

[openjdk]

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

8261675: ObjectValue::set_visited(bool) sets _visited false

Reviewed-by: kvn

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

• e7e20d4: 8261711: Clhsdb "versioncheck true" throws NPE every time
• 55d7bbc: 8261607: SA attach is exceeding JNI Local Refs capacity
• 0a50688: 8241372: Several test failures due to javax.net.ssl.SSLException: Connection reset
• 61a659f: 8260415: Remove unused class ReferenceProcessorMTProcMutator
• 6b6f794: 8248223: KeyAgreement spec update on multi-party key exchange support
• 8ba390d: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702
• 16bd7d3: 8261336: IGV: enhance default filters
• 3f8819c: 8261501: Shenandoah: reconsider heap statistics memory ordering
• 3cbd16d: 8259668: Make SubTasksDone use-once
• 219b115: 8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
• ... and 71 more: https://git.openjdk.java.net/jdk/compare/4619f372ae5934091b0d40621a1dbcd9e4b0f80c...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 (@vnkozlov) 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).

[navyxliu]

/integrate

[openjdk]

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

[navyxliu]

Hi, Vladimir,

I am fine with your current change (getter and setter). Let push it.
And I will work on suggested optimization/clean in separate RFE (change will need comments to avoid confusion, as you have).

Sounds good. Thank you for sponsoring it.

[vnkozlov]

/sponsor

[vnkozlov]

Note, I think it is trivial change and one review is enough.

[openjdk]

@vnkozlov @navyxliu Since your change was applied there have been 81 commits pushed to the master branch:

• e7e20d4: 8261711: Clhsdb "versioncheck true" throws NPE every time
• 55d7bbc: 8261607: SA attach is exceeding JNI Local Refs capacity
• 0a50688: 8241372: Several test failures due to javax.net.ssl.SSLException: Connection reset
• 61a659f: 8260415: Remove unused class ReferenceProcessorMTProcMutator
• 6b6f794: 8248223: KeyAgreement spec update on multi-party key exchange support
• 8ba390d: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702
• 16bd7d3: 8261336: IGV: enhance default filters
• 3f8819c: 8261501: Shenandoah: reconsider heap statistics memory ordering
• 3cbd16d: 8259668: Make SubTasksDone use-once
• 219b115: 8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
• ... and 71 more: https://git.openjdk.java.net/jdk/compare/4619f372ae5934091b0d40621a1dbcd9e4b0f80c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2677f6f.

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