RFR: 8201318: Introduce GCThreadLocalData to abstract GC-specific data belonging to a thread

[Per Liden]

Hi Aleksey,

On 04/11/2018 12:13 PM, Aleksey Shipilev wrote:
> On 04/11/2018 11:54 AM, Per Liden wrote:
>>> *) Should these new methods accept JavaThread*? I.e. we are expecting the GC thread-local data only
>>> for JavaThreads, like attach/detach does?
>>>
>>>      virtual void on_thread_create(Thread* thread);
>>>      virtual void on_thread_destroy(Thread* thread);
>>>      virtual void on_thread_attach(JavaThread* thread);
>>>      virtual void on_thread_detach(JavaThread* thread);
>>
>> The intention is that on_thread_create/destroy is called for all threads, where as
>> on_thread_attach/detach is only called for JavaThreads (i.e. threads that are added to the "threads
>> list"). There's been discussions about treating all threads in the same way, i.e. they would all be
>> on the threads list and they would all receive attach/detach calls. It's not clear yet if that is a
>> good idea, but if that change is made in the future, then we might want to revisit this interface.
> 
> Ah, I understand. After sending the reply yesterday I realized they might be legit use cases where
> you want to have GCTLD available e.g. for GC threads.
> 
>> As discussed else where in this thread, the reason for keeping the GCThreadLocalData member early in
>> Thread is to allow GC barriers to generate compact code. As Erik indicates, this may not matter that
>> much in the end, but I also don't think it hurts to let it sit there.
>>
>> Also, I've shrunk GCThreadLocalData to 112 bytes, which is what G1 currently needs. ZGC currently
>> uses 156 bytes (IIRC), but that's a change that should stay the ZGC repo for now.
> 
> Oh. Haven't expected we need that much. This makes me think if we should implement some sort of
> STATIC_ASSERT-based overflow checks? Not necessarily in this RFE.

Actually, it's already there, in Thread::gc_data().

> 
>> Updated webrev here:
>>
>> Diff: http://cr.openjdk.java.net/~pliden/8201318/webrev.1vs2/
>> Full: http://cr.openjdk.java.net/~pliden/8201318/webrev.2/
> 
> Looks good!
> 
> Minor nits (no need to re-review):
> 
> *) I'll probably do another sweep and do a tad better indenting work. Compare:
> 
> 2178   Address in_progress(Rthread, in_bytes(G1ThreadLocalData::satb_mark_queue_active_offset()));
> 2179   Address index(Rthread, in_bytes(G1ThreadLocalData::satb_mark_queue_index_offset()));
> 2180   Address buffer(Rthread, in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset()));
> 
> vs.
> 
> 2178   Address in_progress(Rthread, in_bytes(G1ThreadLocalData::satb_mark_queue_active_offset()));
> 2179   Address       index(Rthread, in_bytes(G1ThreadLocalData::satb_mark_queue_index_offset()));
> 2180   Address      buffer(Rthread, in_bytes(G1ThreadLocalData::satb_mark_queue_buffer_offset()));
> 
> *) Typos: "provied" -> "provided", "over head" -> "overhead".
> 
>   // Thread local data area for GC-specific information. Each GC
>   // is free to decide the internal structure and contents of this
>   // area. It is represented as a 64-bit aligned opaque blob to
>   // avoid circular dependencies between Thread and all GCs. For
>   // the same reason, the size of the data area is hard coded to
>   // provied enough space for all current GCs. Adjust the size if
>   // needed, but avoid making it excessively large as it adds to
>   // the memory over head of creating a thread

Oops! Will fix.

> 
> *) Also mention the choice of <128 bytes? Like this:
> 
>   // The size of GCTLD is kept under 128 bytes to let compilers encode
>   // accesses to GCTLD members with short offsets from the Thread.

Hmm, I don't quite think this captures the crux of this. To optimize for 
code compactness, you just need to make sure that the data referenced by 
generated barriers is kept within a 127 byte offset. For example, in ZGC 
we only ever reference the first word of the GCTLD in the generated 
barriers (i.e. well below 127). The rest of the data is used when we hit 
the slow path, and then we're executing in the VM and don't care about 
the offset. So, from this point of view, the GCTLD could have any size 
and it wouldn't matter for code compactness. Same it true for G1 and 
Shenandoah, i.e. their generated barriers would be unaffected by an 
increase in GCTLD size.

Thanks!
/Per

> 
> 
> -Aleksey
>