Re: [15] RFR 8238633: JVMTI heap walk should consult GC for marking oops

[Zhengyu Gu]

The JFR heap walker does use the shared barriers in the safepoint though. So 
that optimization sounds like it won’t work.

Okay.


Having said that, the null check will be taken only for runtime code, not when 
going through the JIT. I would be surprised if this very well predicted NULL 
check used by runtime code would be noticeable, especially since you are 
probably going to CAS as well in the same path this is taken (the mark word is 
”marked”).

So perhaps just adding the NULL check in the barrier for the case where the 
markWord ”is_marked” is the sane thing to do, knowing that the other costs 
taken in the same path will dominate.

I have this patch, exactly what you suggested. I will let Aleksey run 
his numbers.

diff -r ef1e608a5ecc 
src/hotspot/share/gc/shenandoah/shenandoahForwarding.inline.hpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahForwarding.inline.hpp 
Mon Feb 24 15:03:28 2020 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahForwarding.inline.hpp 
Mon Feb 24 12:56:08 2020 -0500
@@ -37,10 +37,12 @@
 inline HeapWord* ShenandoahForwarding::get_forwardee_raw_unchecked(oop 
obj) {
   markWord mark = obj->mark_raw();
   if (mark.is_marked()) {
-    return (HeapWord*) mark.clear_lock_bits().to_pointer();
-  } else {
-    return cast_from_oop<HeapWord*>(obj);
+    HeapWord* fwdptr = (HeapWord*)mark.clear_lock_bits().to_pointer();
+    if (fwdptr != NULL) {
+      return fwdptr;
+    }
   }
+  return cast_from_oop<HeapWord*>(obj);
 }

Thanks,

-Zhengyu


Thanks,
/Erik

Thank,

-Zhengyu

Thanks,
/Erik
On 24 Feb 2020, at 17:49, Zhengyu Gu <z...@redhat.com> wrote:

Hi all,

Updated according to your comments:
http://cr.openjdk.java.net/~zgu/JDK-8238633/webrev.02/

I modified vmTestbase/nsk/jvmti/unit/heap/HeapWalkTests/TestDescription.java 
test [1] to walk 300K objects.

Without patch:
Time: 987431 nsecs
Time: 1135390 nsecs
Time: 1142519 nsecs
Time: 962816 nsecs
Time: 1015958 nsecs

Avg: 1048822 nsecs

With patch:
1105015 nsecs
1142425 nsecs
968057 nsecs
1383838 nsecs
1079885 nsecs

Avg: 1135844 nsecs

So, it shows about 8% performance hit.

Thanks,

-Zhengyu

[1] http://cr.openjdk.java.net/~zgu/JDK-8238633/test/webrev.00/





On 2/21/20 8:01 AM, coleen.phillim...@oracle.com wrote:
Adding serviceability-dev back.
Coleen
On 2/21/20 7:59 AM, coleen.phillim...@oracle.com wrote:

Hi, I had a quick look at this, minus the shenandoah code.

http://cr.openjdk.java.net/~zgu/JDK-8238633/webrev.01/src/hotspot/share/gc/shared/objectMarker.hpp.html

I think this file could have forward declarations of GrowableArray and I didn't 
see a need for the markWord.hpp include.

This change on the whole looks good to me.

Coleen

On 2/21/20 5:23 AM, Stefan Karlsson wrote:
Hi Zhengyu,

On 2020-02-17 15:51, Zhengyu Gu wrote:
Hi Stefan,

Thanks for the review and suggestions, updated accordingly:

http://cr.openjdk.java.net/~zgu/JDK-8238633/webrev.01/

Thanks for moving the code. I think this looks good.

If you're up for it, I have a couple of style change suggestions:

1) ObjectMarker uses two verbs to describe the same thing: "mark" and "visit". I propose that we 
only use "mark" in ObjectMarker and leave the usage of "visited" to the Jvmti code.

2) Some updates to odd whitespaces

3) Using forward declarations in Shenandoah code.

I've bundled those changes into webrevs:

https://cr.openjdk.java.net/~stefank/8238633/webrev.01.delta
https://cr.openjdk.java.net/~stefank/8238633/webrev.01

Regarding performance testing, the HeapWalkTests you used seems to use a very 
small heap. I think it would be good to redo the measurements on a larger heap. 
Could you take the HeapWalkTest and add a few GBs of small, linked objects?

Thank,
StefanK


---
Previously, the calls to 'mark' and 'visited' were inlineable, but now every GC 
has to take a virtual call when marking the objects. My guess is that this code 
is slow anyway, and that it doesn't matter too much, but did you measure the 
effect of that change with, for example, G1?

I did rough measurement, timing 
vmTestbase/nsk/jvmti/unit/heap/HeapWalkTests/TestDescription.java test.

If you know any tests/benchmarks I should measure, please let me know.

Thanks,

-Zhengyu


Thanks,
StefanK

Test:
    hotspot_gc
    vmTestbase_nsk_jdi
    vmTestbase_nsk_jvmti

Thanks,

-Zhengyu