RFR (S): 8183226: Remembered set summarization accesses not fully initialized java thread DCQS

[Erik Helin]

On 07/03/2017 02:12 PM, Thomas Schatzl wrote:
> Hi all,

Hi Thomas,

>   can I get reviews for the following change that breaks some
> dependency cycle in g1remset initialization to fix some (at this time
> benign) bug when printing remembered set summarization information?
>
> The problem is that G1Remset initializes its internal remembered set
> summarization helper data structure in the constructor, which accesses
> some DCQS members before we call the initialize methods on the various
> global DCQS'es in G1CollectedHeap::initialize().
> By splitting the initialization of the remembered set summarization
> into an extra method, this one can be called at the very end of
> G1CollectedHeap::initialize(), thus breaking the dependency.

I think there is an easier way to achieve this :) The default 
constructor for G1RemSetSummary sets up almost all fields, and we make 
it really set up _all_ fields, then I believe we are good:
- G1RemSetSummary::_num_vtimes can be set up in the constructor,
   because the number of entries only depends on
   ConcurrentG1Refine::thread_num(), which is a static function that
   only return G1ConcRefinementThreads.
- G1RemSetSummary::_rs_threads_vtimes can be allocated in the
   constructor.
- The value for _rs_threads_vtimes can be initialized to 0, since the
   accumulated virtual time for the each concurrent refinement thread
   should be 0 (since they haven't even started yet).
- Same reasoning as above goes for _sampling_thread_vtime.
- _rem_set can be NULL

With the above changes, G1RemSet will call the default constructor (same 
as it currently does). The call to _prev_period_summary.initialize() 
will be removed from the G1RemSet constructor.

With the above changes, G1RemSetSummary::G1RemSetSummary() has no 
dependencies on any other class, and is still initialized to the correct 
values. I think this is all that is needed to solve this problem.

The rest, below this line, is just existing code that could really 
benefit from a cleanup :)

The G1RemSetSummary::initialize method is no longer needed, 
G1RemSetSummary can now instead have a constructor taking a G1RemSet* as 
argument. That constructor will do what G1RemSetSummary::initialize does 
today.

In G1RemSet::print_periodic_summary_info, the code can then look like:

   G1RemSetSummary current(this);
   _prev_period_summary.subtract(&current);

For extra, extra bonus points, we should make 
G1RemSetSummary::subtract_from work the other way around, so that the 
above code reads:

   G1RemSetSummary current(this);
   current.subtract(_prev_period_summary); // current -= prev

instead of what the code does today:

   prev.subtract_from(current); // prev = current - prev

which to me reads completely backwards :)

Finally, it would be very nice for G1RemSetSummary to get a proper copy 
constructor, so that the last line in print_periodic_summary:

   _prev_period_summary.set(&current);

can just become:

   _prev_period_summary = current;

(G1RemSetSummary::set is just a copy-constructor in disguise)

You don't need to do all the cleanups, but I think having a fully 
functioning default constructor is a better way to solve this problem, 
rather than shuffling the call to initialize around. What do you think?

Thanks,
Erik

> Benign because the values accessed at that time have the same values as
> the values after initialization.
>
> This also allows for grouping together the initialization of
> G1RemSet/DCQS/G1ConcurrentRefine related data structures more easily in
> G1CollectedHeap::initialize().
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8183226
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8183226/webrev/
> Testing:
> local testing running remembered set summarization manually, jprt
>
> Thanks,
>   Thomas
>