RFR (S): 8183226: Remembered set summarization accesses not fully initialized java thread DCQS
erik.helin at oracle.com
Fri Jul 7 11:16:40 UTC 2017
On 07/03/2017 02:12 PM, Thomas Schatzl wrote:
> Hi all,
> 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
- 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
In G1RemSet::print_periodic_summary_info, the code can then look like:
For extra, extra bonus points, we should make
G1RemSetSummary::subtract_from work the other way around, so that the
above code reads:
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:
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?
> 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
> local testing running remembered set summarization manually, jprt
More information about the hotspot-gc-dev