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

[Erik Helin]

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/

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.

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
>