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

[Stefan Karlsson]

Hi Zhengyu,

On 2020-02-07 16:53, Zhengyu Gu wrote:
> Hi,
> 
> I would like purpose this change that allows GC to provide ObjectMarker 
> during JVMTI heap walk.
> 
> Currently, JVMTI heap walk uses oop markword's 'marked' pattern to 
> indicate 'visited' oop.
> 
> Unfortunately, it conflicts with Shenandoah, who uses the pattern to 
> indicate 'forwarding'. When JVMTI heap walk occurs in some of 
> Shenandoah's concurrent heap (e.g. concurrent evacuation or concurrent 
> reference updating phases), it can result corrupted heap, as it tries to 
> resolve a real oop header as a forwarding pointer.
> 
> This patch allows GC to provide ObjectMarker for JVMTI to track 
> 'visited' oop, and uses current implementation as default, so that, it 
> has no impact to GCs other than Shenandoah, who provides its own 
> implementation.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8238633
> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8238633/webrev.00/index.html
>


Would you mind if I asked you to move the object marker code to its own 
objectMarker.hpp/cpp files?

---
Another suggestion is to move the "marking" out of the visit() function, 
and renamed ObjectMarker::visted() to ObjectMarker::marked().

-      if (!ObjectMarker::visited(o)) {
-        if (!visit(o)) {
+      if (!marker.object_marker()->visited(o)) {
+        if (!visit(o, marker.object_marker())) {

Would become:
+      if (!marker.object_marker()->mark(o)) {
+        if (!visit(o)) {

This assert would be unnecessary:
-bool VM_HeapWalkOperation::visit(oop o) {
+bool VM_HeapWalkOperation::visit(oop o, ObjectMarker* object_marker) {
    // mark object as visited
-  assert(!ObjectMarker::visited(o), "can't visit same object more than 
once");
-  ObjectMarker::mark(o);
+  assert(!object_marker->visited(o), "can't visit same object more than 
once");
+  object_marker->mark(o);

The name and comment would match:
-// return true if object is marked
-inline bool ObjectMarker::visited(oop o) {
-  return o->mark().is_marked();
-}

---
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?

Thanks,
StefanK

> Test:
>    hotspot_gc
>    vmTestbase_nsk_jdi
>    vmTestbase_nsk_jvmti
> 
> Thanks,
> 
> -Zhengyu
> 
>