RFR 8221435: Shenandoah should not mark through weak roots

[Zhengyu Gu]

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

On 3/26/19 1:21 PM, Aleksey Shipilev wrote:
> On 3/26/19 6:05 PM, Zhengyu Gu wrote:
>> Please review this patch that fixes incorrectly marking through weak roots in Shenandoah.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221435
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8221435/webrev.00/index.html
> 
> *) Please qrefresh the changeset comment
> 
> *) shenandoahConcurrentMark.cpp, indenting at L660..L661
> 
>   659   if (_heap->has_forwarded_objects()) {
>   660       ShenandoahWeakUpdateClosure cl;
>   661       WeakProcessor::weak_oops_do(workers, is_alive.is_alive_closure(), &cl, 1);
>   662   } else {
>   663     ShenandoahWeakAssertNotForwardedClosure cl;
>   664     WeakProcessor::weak_oops_do(workers, is_alive.is_alive_closure(), &cl, 1);
>   665   }

Fixed.

> 
> *) 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.

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