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

[Thomas Schatzl]

Hi Erik (and Stefan),

  thanks for your review.

On Fri, 2017-07-07 at 13:16 +0200, Erik Helin wrote:
> 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.

Fine with me.

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

Unfortunately, no. We can't pass "this" in the constructor as the
compilers will complain about possible use of uninitialized class (or
so).

But we can always get the G1RemSet pointer from the global variables,
as implemented.

> 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 :)
> 

I think there has been some reason I do not remember right now why this
has been done that way. But I agree.

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

I remember trying to avoid adding a copy constructor for fear of being
"too complicated" and unusual for Hotspot code.

> 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?

Let's defer the other suggested cleanups to a different CR.

In the following webrev I also added StefanJ's suggestion to extract
concurrent refinement initialization into a separate method.
(I do not really understand why that method is actually returning an
error code: all error conditions in ConcurrentG1Refine call
vm_shutdown_during_initialization() anyway - even that seems
superfluous: failing to allocate memory shuts down the VM already).

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8183226/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8183226/webrev.1/ (full)

Thanks,
  Thomas