RFR: 8230566: ZGC: Don't substitute klass pointer during array clearing

[Erik Österlund]

Hi Per,

In  src/hotspot/share/gc/z/zHeapIterator.cpp the only change made here 
is to mess up the indentation:

  195   // Push roots to visit
  196   push_roots<ZRootsIterator,                    false /* Concurrent */, false /* Weak */>();
  197   push_roots<ZConcurrentRootsIteratorClaimOther, true  /* Concurrent */, false /* Weak */>();

Should probably change that back.

One possible solution to the problem of passing around all this context information in the templates,
is to wrap them in a template config type, like this:

template <boolfollow, bool finalizable, bool publish>
struct ZMarkConfig {
   static const bool _follow = follow;
   static const bool _finalizable = finalizable;
   static const bool _publish  = publish;
};

Then you can just pass around the ZMarkConfig in the templates all over the place, instead of the 3 (at this
moment) bools. That way, adding and removing bools becomes easier in the future; no need to change the passing
around code to pass more things around. Only the producer and consumer of the flags need changing.

However, I'd propose we do that in a follow-up RFE so we can fix the bug faster. Having said that, looks good,
and don't need a new webrev.

Thanks,
/Erik

On 2019-09-04 14:02, Per Liden wrote:
> When allocating large object arrays, ZObjArrayAllocator instantiates a 
> non-cleared array of longs and later installs the correct klass 
> pointer once the array has been cleared. While this might work it's 
> also error prone. For example, there are at least two asserts() that 
> will re-loaded the klass pointer and fail. It's also easy for someone 
> in the future to make the innocent mistake of re-loading the klass 
> pointer in some sensitive path, which would lead to new problems. We 
> can avoid all these problems by not substituting with the klass 
> pointer, and instead have another mechanism to tell GC marker threads 
> to not follow the elements in not-yet-cleared arrays.
>
> This patch adds a new bit to ZMarkStackEntry, to signal to marking 
> threads that the object array should not be followed, just mark it as 
> live and visit its klass.
>
> The patch is large-ish because of the need to propagate the "follow" 
> flag down through a few of layers. We might want to think about 
> converting the flags (follow/finalizable/publish) into a single 
> ZBarrierFlags-thing (perhaps similar to DecoratorSet) to reduce the 
> noise and make it easier to add/adjust/remove flags in the future. But 
> such an enhancements could come later.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8230566
> Webrev: http://cr.openjdk.java.net/~pliden/8230566/webrev.0
>
> Testing: tier1-6 on linux + ad hoc runs of tests that provoked the 
> problem
>
> /Per