RFR (M) 8235573: Move JFR ObjectSample oop into OopStorage

[coleen.phillimore at oracle.com]

Kim, Thank you for reading this change.

On 8/1/20 5:14 AM, Kim Barrett wrote:
>> On Jul 31, 2020, at 11:43 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> Markus contributed a better incremental patch on top of this, please review tested 02 patch instead.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/2020/8235573.02/webrev
>>
>> Thanks,
>> Coleen
>>
>> On 7/31/20 10:16 AM, coleen.phillimore at oracle.com wrote:
>>> Summary: Add a weak OopStorage for JFR and move the oops there. Remove GC code to process oops.
>>>
>>> Tested with tier1,5, and 6.  Built with shenandoah.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/2020/8235573.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8235573
>>>
>>> Thanks,
>>> Coleen
> One less serial weak processing phase!  Yay!

We thought you'd like that.  Unfortunately, the jvmti serial phase is 
hard to eradicate.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp
>    66     if (!_dead_samples) {
>    67       _dead_samples = true;
>    68     } else {
>    69       // Need exclusive access to avoid a race that could loose a
>    70       // notification about new dead samples. Previously, the notification
>    71       // happened during a safepoint, i.e. as part of weak_oops_do(),
>    72       // but with the change to OopStorage, it can happen concurrently
>    73       // with threads trying to reset the flag.
>    74       acquire();
>    75       _dead_samples = true;
>    76       release();
>    77     }
>
> _dead_samples is potentially accessed by multiple threads, including
> writers.  That suggests it needs to be an "atomic" variable.
>
> Blocking on a potentially long-duration (spin!) lock in this gc-callback
> might be a problem.  The approach taken by the string table might be better.
> There is a similar race there.  The approach taken there is to just lose the
> race and let the next GC trigger cleanup.  But StringTable also uses another
> thread (the ServiceThread) to perform the cleanup, rather than piggy-backing
> cleanup on the add() operation, which might not occur at an appropriately
> timed point.
>
> I think a structure similar to that used by the StringTable would work well
> here too.
>
> A different approach would be to have the gc-callback unconditionally set
> _dead_samples if any are reported, and have scavenge clear _dead_samples
> before doing its cleanup.  But that can result in back-to-back cleanups
> (by successive adds) where the second might not be needed.  The StringTable
> approach prefers a possibly delayed cleanup vs unnecessary cleanups.  And
> it seems a delayed cleanup is acceptable here, since it won't even be
> considered until an add operation following _dead_samples being set.

Markus and I talked about this and since the lists are generally short 
and the race is very unlikely, having back-to-back GCs is a problem that 
this can live with.  I'd hate to see the JFR code spashed into the 
ServiceThread mechanism for this race.

I've also made all the _dead_samples accesses atomic, and set it 
unconditionally in the gc_notification.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp
>   100   _size(size) {
>   101     _last_sweep = JfrTicks::now();
>   102     _dead_samples = false;
>   103 }
>
> Assignments in the body are mis-indented.  But why were they changed from
> mem-initializers to assignments?

_last_sweep and _dead_samples are statics now, so aren't mem initializers.

I fixed the indentation.
>
> ------------------------------------------------------------------------------
>
> Including "Weak" in the oopstorage's name would be consistent with the
> others, and makes things a little more obvious in GC logs.
>
>

Fixes this too.

incremental webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8235573.02.incr/webrev
full webrev at http://cr.openjdk.java.net/~coleenp/2020/8235573.03/webrev

And retested.
Thanks,
Coleen