RFR 8221435: Shenandoah should not mark through weak roots

[Zhengyu Gu]

On 3/26/19 3:31 PM, Roman Kennke wrote:
> 
> 
>>> *) I think the WeakProcessor::weak_oops_do is deliberately called 
>>> with serial version when
>>> ShenandoahWeakAssertNotForwardedClosure is handled. Maybe that 
>>> prolongs weak root handling
>>> unnecessarily? Anyhow, this comment needs to be moved to new code:
>>
>> I was puzzled by this also. It may be the case in early days, when 
>> there are few oop storages? seems we definitely can benefit parallel 
>> version, because there are more oop storages to process.
>>
>> Roman, do you recall the reason why it was done this way?
>>
>> ShenandoahWeakAssertNotForwardedClosure is debug only closure, 
>> surround the body with ifdef.
> 
> Well, as you say, it's debug-only, so shouldn't matter performance-wise. 
> Except, as you say, it should be #ifdef ASSERT. It wasn't #ifdef'd 
> before? WTF?
> 
> Other than that, looks good. Maybe we should wait until we have fixes for:
> 
> https://bugs.openjdk.java.net/browse/JDK-8221102
> and
> https://bugs.openjdk.java.net/browse/JDK-8220671

Okay, I can hold off for them.

-Zhengyu

> 
> in sh/jdk, so that we have a stable testing base?
> 
> Roman
> 
>>>   709     // Process leftover weak oops: update them, if needed 
>>> (using parallel version),
>>>   710     // or assert they do not need updating (using serial 
>>> version) otherwise.
>>>   711     // Weak processor API requires us to visit the oops, even 
>>> if we are not doing
>>>   712     // anything to them.
>>>
>>> *) The distinction between 
>>> ShenandoahRootProcessor::process_all_roots, ::update_all_roots,
>>> ::traversal_update_all_roots might be better? (I cannot see the 
>>> better factoring right away, though).
>>>
>> There are inconsistencies in traversal, I am looking into them in 
>> follow up RFE, will address this there.
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>> -Aleksey
>>>