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

[Aleksey Shipilev]

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.

> 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

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


-Aleksey