RFR: 8219613: Use NonJavaThread PtrQueues

[zgu at redhat.com]

Sorry for the confusion. As I stated early, my comments are not
directly related to this RFR. By no means, it should block this patch.

On Tue, 2019-02-26 at 23:58 +0100, Aleksey Shipilev wrote:
> On 2/26/19 11:35 PM, Kim Barrett wrote:
> > > On Feb 26, 2019, at 5:10 PM, Kim Barrett <kim.barrett at oracle.com>
> > > wrote:
> > > > On Feb 26, 2019, at 10:58 AM, zgu at redhat.com wrote:
> > > > Probably not directly relates to this RFR, but I see a couple
> > > > of issues
> > > > with this BarrierSet::on_thread_xxx mechanism.
> > > > 
> > > > 1) When creating worker thread, it is added to WorkGang's
> > > > thread first,
> > > > and on_thread_attach() is not called until worker thread starts
> > > > to run.
> > > > If I use on_thread_attach() to initialize something (e.g.
> > > > gclab), I may
> > > > hit this timing gap, because WorkGang::threads_do() may hit
> > > > uninitialized data (e.g. gclab)
> 
> I think this is resolved specially in Shenandoah::post_initialize,
> when GC workers get their initial
> GCLABs. Zhengyu, can you confirm this?

Yes. 

> 
> > > ShenandoahHeap::make_parsable and
> > > ShenandoahHeap::retire_and_reset_gclabs seem to be looking at the
> > > gclab data of non-Java threads, and going to some effort to do
> > > so.
> > > But the per-thread gclab info only gets initialized for Java
> > > threads.
> > > Neither of those are changed by this patch.
> > > 
> > > In pre-RFR discussion with Aleksey about the Shenandoah part of
> > > this
> > > patch, he said non-Java threads don't use the gclab mechanism, so
> > > only
> > > initializing the per-thread gclab data for Java threads was (and
> > > remains) correct.  But make_parsable and retire_and_reset_gclabs
> > > are
> > > looking at that data anyway.
> 
> I think this is still correct. Worker threads are indeed non-Java
> threads, but they are handled
> specially in ShenandoahWorkGang::install_worker, where their gclab is
> getting initialized. This
> "exception" is why make_parsable, retire_and_reset_gclab and friends
> are applying closure to GC
> worker threads. And why ShenandoahInitGCLABClosure applies to all
> threads, and then filter by
> (thread->is_Java_thread() || thread->is_Worker_thread()) -- this is
> needed to resolve some
> initialization races.

Yes, that's how Shenandoah currently works. However, I feel we should
ought to be able to use on_thread_xxx mechanism to replace it, cause it
sounds more straight forward.


> 
> > Also, ShenandoahHeap::post_initialize applies the
> > ShenandoahInitGCLABClosure
> > to "all" threads (via Threads::threads_do), and some worker threads
> > (via WorkGang::threads_do on the various work gangs), which will
> > double call the closure (unless one is "lucky" and hits the race.)
> 
> Oh, double-visiting is not nice indeed! Need to fix that, thanks.
> This issue does not block this patch.
Yes, I am working on this. and yes, it should not block this patch.

Thanks,

-Zhengyu

> 
> -Aleksey
>