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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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

-------------

Commit messages:
 - 8261675: ObjectValue::set_visited(bool) sets _visited false

Changes: https://git.openjdk.java.net/jdk/pull/2560/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2560&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261675
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2560.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2560/head:pull/2560

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov-2
On Sat, 13 Feb 2021 01:51:39 GMT, Xin Liu <[hidden email]> wrote:

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

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](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/code/debugInfoRec.cpp#L358)

`is_visited()` is not called at all - `_visited` field is accessed directly only in one place:
[debugInfo.cpp#L161](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/code/debugInfo.cpp#L161)

Would be nice to clean up this mess.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Kozlov-2
On Sat, 13 Feb 2021 06:33:36 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> The setter is error-prone. it unconditionally sets _visited false.
>> this patch stores the argument to it.
>
> 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](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/code/debugInfoRec.cpp#L358)
>
> `is_visited()` is not called at all - `_visited` field is accessed directly only in one place:
> [debugInfo.cpp#L161](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/code/debugInfo.cpp#L161)
>
> Would be nice to clean up this mess.

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](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/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);
     }
   }

-------------

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

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

Xin Liu
On Sat, 13 Feb 2021 08:10:22 GMT, Vladimir Kozlov <[hidden email]> wrote:

> 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](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261675: ObjectValue::set_visited(bool) sets _visited false [v2]

Xin Liu
In reply to this post by Xin Liu
> The setter is error-prone. it unconditionally sets _visited false.
> this patch stores the argument to it.

Xin Liu has updated the pull request incrementally with one additional commit since the last revision:

  8261675: ObjectValue::set_visited(bool) sets _visited false
 
  use getter and setter of _visited.
  update the year of copyright.

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2560/files
  - new: https://git.openjdk.java.net/jdk/pull/2560/files/2d512adc..dce5bbf2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2560&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2560&range=00-01

  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2560.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2560/head:pull/2560

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261675: ObjectValue::set_visited(bool) sets _visited false [v2]

Vladimir Kozlov-2
On Sat, 13 Feb 2021 21:46:56 GMT, Xin Liu <[hidden email]> wrote:

>> The setter is error-prone. it unconditionally sets _visited false.
>> this patch stores the argument to it.
>
> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
>
>   8261675: ObjectValue::set_visited(bool) sets _visited false
>  
>   use getter and setter of _visited.
>   update the year of copyright.

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).

-------------

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261675: ObjectValue::set_visited(bool) sets _visited false [v2]

Xin Liu
On Tue, 16 Feb 2021 22:40:01 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Xin Liu has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8261675: ObjectValue::set_visited(bool) sets _visited false
>>  
>>   use getter and setter of _visited.
>>   update the year of copyright.
>
> 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).

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261675: ObjectValue::set_visited(bool) sets _visited false [v2]

Vladimir Kozlov-2
On Tue, 16 Feb 2021 23:56:18 GMT, Xin Liu <[hidden email]> wrote:

>> 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).
>
> 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.

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2560
Reply | Threaded
Open this post in threaded view
|

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

Xin Liu
In reply to this post by Xin Liu
On Sat, 13 Feb 2021 01:51:39 GMT, Xin Liu <[hidden email]> wrote:

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

This pull request has now been integrated.

Changeset: 2677f6f4
Author:    Xin Liu <[hidden email]>
Committer: Vladimir Kozlov <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/2677f6f4
Stats:     4 lines in 2 files changed: 0 ins; 0 del; 4 mod

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

Reviewed-by: kvn

-------------

PR: https://git.openjdk.java.net/jdk/pull/2560