Potential ShTraversalGC::weak_refs_work_doit bug

[Roman Kennke]

> On 09/10/2018 11:38 AM, Roman Kennke wrote:
>>> This block has some weirdness:
>>>
>>> http://hg.openjdk.java.net/shenandoah/jdk/file/e60d7f4b3ba9/src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp#l1164
>>>
>>> *) is_alive is redefined -- to the same value?
>>
>> Where do you see is_alive redefined? It is defined once here:
>> http://hg.openjdk.java.net/shenandoah/jdk/file/e60d7f4b3ba9/src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp#l1172
>>
>>
>>> *) complete_gc is not used? Shouldn't we call it to pick up new stuff from weak_oops_do?
>>
>> It is used in WeakProcessor::weak_oops_do() calls, isn't it?
>>
>> Or maybe I misunderstand your questions?
> 
> It really helps to use the proper IDE that highlights those things ;)
>   http://cr.openjdk.java.net/~shade/shenandoah/clion-traversal-rp.png
> 
> -Aleksey
> 
> 

The answer is that the objects processed by WeakProcessor don't span any
new object sub-graph that require additional marking, and hence no
complete_gc closure is needed.

Even the keep_alive closure is mis-understood by us: Stuff processed by
WeakProcessor is 'weak' in the sense that after marking is completed
(i.e. all strong, weakrefs, etc are traversed), it is decided based on
the is_alive closure, which objects/refs to keep and which to clean. The
keep_alive closure is not meant to make the GC keep objects alive (e.g.
by marking), but only as callback to let the GC know that the object has
been kept alive, in case the GC need to take action on this. For
Shenandoah this only means to update the reference to point to to-space
when we're marking+updating-refs in single phase, otherwise it can do
nothing. Traversal also needs to update-refs there.

Aleksey: can you look at streamlining this? Also, can you make it use
the MT-version of WeakProcessor?

Thanks,
Roman