RFR: JDK-8134953: Make the GC ID available in a central place

[Bengt Rutisson]

Hi again,

I found an issue with how G1 young collections kick off concurrent 
marks. There is a short window where we might have two active GC IDs at 
the same time. I've fixed this and updated the webrevs. In case anyone 
had already started looking at the webrevs you need to do a refresh now. 
The fix is in G1CollectedHeap::do_collection_pause_at_safepoint().

Thanks,
Bengt

On 09/09/15 13:38, Bengt Rutisson wrote:
>
> Hi Per,
>
> Thanks for looking at this!
>
> On 2015-09-08 15:23, Per Liden wrote:
>> Hi Bengt,
>>
>> On 2015-09-08 13:35, Bengt Rutisson wrote:
>>>
>>> Hi everyone,
>>>
>>> This is mostly a GC related patch, but it adds a field to the Thread
>>> class, so I'm sending this out on the broader hotspot-dev list.
>>>
>>> Could I have a couple of reviews for this patch?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8134953/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8134953
>>>
>>
>> Looks good. I think this is a nice simplification, especially for 
>> G1's concurrent parts. Just two comments:
>>
>> genCollectedHeap.cpp:
>> ---------------------
>> - GenCollectedHeap::do_collection() has two GCIdMarks, in different 
>> scopes. Do we really want that second mark?
>
>
> We potentially do two GCs in GenCollectedHeap::do_collection(). First 
> a young GC and then potentially a full GC. These two should have 
> different GC IDs. So, yes, we need two GCIdMarks in this method. The 
> scoping could be better, but I think that is a refactoring that should 
> be done separately from my current patch. I'll talk to Jesper about it 
> since he has been cleaning up this code lately.
>
>>
>>
>> gcId.cpp:
>> --------
>> - I think you might have left currentNamedthread() in there. You 
>> probably just want to use Thread::current() instead.
>
> Yes, good catch. Thanks.
>
> However, after thinking some more about the implications of moving the 
> GC ID out from the GCTracer I figured it would be possible to just 
> store the previous GC ID in the GCIdMark and then restore it in the 
> destructor of that class. That way we don't have to store anything in 
> the Thread class but can still have both a concurrent GC ID and a STW 
> GC ID active at the same time. This also removes the need to copy the 
> GC ID to the worker tasks.
>
> Here's an updated webrev with a solution that does not add anything to 
> the Thread class:
>
> http://cr.openjdk.java.net/~brutisso/8134953/webrev.02/
>
> And here's a diff compared to the last version:
>
> http://cr.openjdk.java.net/~brutisso/8134953/webrev.01-02.diff/
>
> Unfotunately the webrev tool had some bad luck when creating the diff 
> webrev so at least the g1CollectedHeap.cpp seem to contain the 
> complete changes rather than just the diff compared to the 01 version. 
> The rest of the diff looks correct as far as I can tell.
>
> Thanks,
> Bengt
>>
>>
>> cheers,
>> /Per
>>
>>
>>> Currently the GC ID, that is used for event tracing and logging, is
>>> stored in the GCTracer object. That has caused some minor problems 
>>> since
>>> there are multiple GCTracers active at the same time. The correct GC 
>>> IDs
>>> need to be passed around in many places.
>>>
>>> For some upcoming GC logging changes I would like to have a more
>>> consistent way of finding the currently active GC ID. I've played 
>>> around
>>> with a couple of different solutions, but eventually I found that the
>>> simplest solution is to store the current GC ID in the thread. That way
>>> there is a single way to look it up in all places. It is also fairly
>>> straight forward to set it up.
>>>
>>> I've reworked the GCId class a bit to support this and I've 
>>> introduced a
>>> GCIdMark class that is a scoped object that helps to set up and tear
>>> down a current GC ID. By moving the GC ID out from the GCTracer class I
>>> got rid of a few minor issues as well. One is that we no longer need to
>>> keep track of the G1 "aborted concurrent GC ID". That was necessary
>>> before since we did logging *after* we had told the corresponding
>>> GCTracer that the concurrent cycle was aborted and it had thrown its GC
>>> ID away. Now the GC ID can stay in scope until we have completed all
>>> logging.
>>>
>>> Also the HeapDumpBeforeFullGC and PrintClassHistogramBeforeFullGC used
>>> to have to create a new GC ID since their logging was done before we 
>>> had
>>> the correct GCTracer set up. Now the GC ID can be available early 
>>> enough
>>> for this logging to be using the same (and correct) GC ID as the 
>>> rest of
>>> the GC. Same for HeapDumpAfterFullGC and 
>>> PrintClassHistogramAfterFullGC.
>>>
>>> I've added an uint to the Thread class to keep track of the currently
>>> active GC ID. In the current code there are actually only NamedThreads
>>> that need the GC ID. So, I'm not sure where the best place is for this
>>> field is. But it seems like the Thread class contains most of the data,
>>> even data that is only used by some subclasses, so I opted for putting
>>> the field in Thread as opposed to in NamedThread. This opens up for
>>> possible future extensions when Java threads may do part of the GC 
>>> work.
>>> However, I'm open to moving the field down to NamedThread if that is
>>> seen as better.
>>>
>>> In the GCTracer class there were many asserts that were checking that
>>> the GC ID was properly set up. Mostly these assert verify that 
>>> start/end
>>> is called correctly and that the other methods are called inside of the
>>> start/end scope. Now that the GC ID is moved out of the GCTracer class
>>> these asserts had to be dealt with. I figured there were three ways to
>>> handle it; just remove them, replace them with check that the GC ID 
>>> from
>>> the current thread is correct, or implement a new status field to keep
>>> track of GC start/end state. Personally I'm not sure these asserts are
>>> valuable enough to spend time on, so I went for the first approach:
>>> removing them. I don't think making them use the thread GC ID will be
>>> appropriate. But if anyone feels strongly about these asserts I can
>>> implement a separate start/end state.
>>>
>>> There are a few "Tasks" in the GC code that are executed by worker
>>> threads. To make the worker threads use the correct GC ID I make sure
>>> that the Task constructors copy the GC ID from the initiating thread
>>> into a local variable. When the task is executed in its worker 
>>> thread it
>>> sets up the GC ID based on the local variable.
>>>
>>> The proposed change does not alter any logging (except for the small 
>>> bug
>>> fix for
>>> HeapDumpBefore(After)FullGC/PrintClassHistogramBefore(After)FullGC. 
>>> This
>>> means that no existing tests need to be updated. In particular the
>>> test/gc/logging/TestGCId.java test passes even after these changes.
>>>
>>>
>>> A big thanks to Per, who pre-reviewed these changes and came with some
>>> really good feedback.
>>>
>>> Thanks,
>>> Bengt
>