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

[Stefan Karlsson]

Talked to Per and Erik offline about follow_array_object. We should be 
able to skip following the klass pointer when follow is false. Just like 
objects in "allocating" regions don't get their klass pointer followed. 
Other than that, this looks good to me.

Thanks,
StefanK



On 2019-09-04 19:15, Per Liden wrote:
> Hi Erik,
> 
> On 9/4/19 5:12 PM, Erik Österlund wrote:
>> 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.
> 
> Will fix!
> 
>>
>> 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.
> 
> I'll prototype and see how it turns out. If it seems to be worth doing 
> I'll file a separate RFE.
> 
> Thanks for reviewing!
> 
> cheers,
> Per
> 
>>
>> 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
>>