RFR: 8036698: Add trace event for updates to metaspace gc threshold

[Mikael Gerdin]

Erik,

On Thursday 06 March 2014 14.06.22 Erik Helin wrote:
> Hi Mikael,
> 
> thanks for reviewing, appreciate it!
> 
> On 2014-03-06 10:59, Mikael Gerdin wrote:
> > Erik,
> > 
> > On 03/06/2014 09:14 AM, Erik Helin wrote:
> >> Hi all,
> >> 
> >> this change adds a new trace event, vm/gc/metaspace/gc_threshold that
> >> tracks updates to the metaspace GC threshold
> >> (MetaspaceGC::_capacity_until_gc).
> >> 
> >> The event contains the old value of MetaspaceGC::_capacity_until_gc, the
> >> new value and also the named of the method that changed the value.
> >> 
> >> Enhancement:
> >> https://bugs.openjdk.java.net/browse/JDK-8036698
> >> 
> >> Webrev:
> >> http://cr.openjdk.java.net/~ehelin/8036698/webrev.00/
> >> 
> >    61 size_t Metaspace::_compressed_class_space_size;
> >    62 const MetaspaceTracer* const Metaspace::_tracer = new
> > 
> > MetaspaceTracer();
> > 
> >    63
> > 
> > I think that in general we try to avoid using static initializers.
> > MetaspaceTracer is a CHeapObj<mtTracing> so on allocation I believe it
> > will attempt to interact with NMT (if enabled).
> > Can you please move the initialization of _tracer to
> > Metaspace::global_initialize
> > I realize this means _tracer will no longer be a *const but I think
> > that's fine compared to static initializer unpredictability.
> 
> Agree, I've updated the patch. Please see new webrev at:
> http://cr.openjdk.java.net/~ehelin/8036698/webrev.01/

Thanks

> 
> On 2014-03-06 10:59, Mikael Gerdin wrote:
> > 3230   size_t after_inc = MetaspaceGC::inc_capacity_until_GC(delta_bytes);
> > 3231   size_t before_inc = after_inc - delta_bytes;
> > 3232
> > 3233   tracer()->report_gc_threshold(before_inc, after_inc,
> > 3234        MetaspaceGCThresholdUpdater::ExpandAndAllocate);
> > 
> > Did you consider changing this to get capacity_until_GC() before
> > incrementing instead of calculating what it was before based on the delta?
> > If we ever change inc_capacity_until_GC to not just accept delta_bytes
> > as-is this code will break.
> 
> Yes, I made this change before StefanK explained to me why this is a
> race condition :)
> 
> If we read capacity_until_GC() before the call to
> inc_capacity_until_GC(), then _another_ thread might update call
> inc_capacity_until_GC() since multiple threads can execute
> expand_and_allocate() at the same time.
> 
> I added a comment in the new version of the patch (see link above) as a
> heads up to anyone else reading this code that might be tempted to do
> this change.

Ok.

/Mikael

> 
> On 2014-03-06 10:59, Mikael Gerdin wrote:
> > I don't really understand the trace*xml changes so I hope someone else
> > has a look at them :)
> 
> No problem, I will just have to find an additional second reviewer for
> trace.xml :)
> 
> Thanks!
> Erik
> 
> > /Mikael
> > 
> >> Testing:
> >> - JFR JTREG tests
> >> - JPRT
> >> 
> >> Thanks,
> >> Erik