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

[Per Liden]

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?


gcId.cpp:
--------
- I think you might have left currentNamedthread() in there. You 
probably just want to use Thread::current() instead.


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