RFR[13]: 8226366: Excessive ServiceThread wakeups for OopStorage cleanup

[coleen.phillimore at oracle.com]

On 7/2/19 2:36 PM, Kim Barrett wrote:
>> On Jul 1, 2019, at 5:36 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> http://cr.openjdk.java.net/~kbarrett/8226366/open.00/src/hotspot/share/runtime/serviceThread.cpp.frames.html
>>
>> Do you have another bug to add the oopStorage for the ResolvedMethodTable to the list?
> JDK-8227053.  Also JDK-8227054.

Good.
>
>> http://cr.openjdk.java.net/~kbarrett/8226366/open.00/src/hotspot/share/runtime/safepoint.cpp.frames.html
>>
>> I suppose you don't need is_safepoint_needed() to trigger this cleanup in the GuaranteedSafepointInterval because if there is no GC, there won't be any blocks to deallocate.
> s/is_safepoint_needed()/is_cleanup_needed()/
>
> There doesn't seem to be a clear theory of what that function should
> check for. Some of the existing safepoint cleanups have checks there,
> some don't, and it's not always obvious why. This cleanup doesn't seem
> so urgent that if there are no other reasons to safepoint for a
> long(ish) time then we should force one for just this purpose.

Yes, it's not well defined.  If something can run forever and need a 
cleanup without a safepoint, it should go in the list.
>
>> http://cr.openjdk.java.net/~kbarrett/8226366/open.00/src/hotspot/share/gc/shared/oopStorage.cpp.frames.html
>>
>> One nit.  The rest of the implementations that do the same thing as this, are called "trigger_concurrent_work".  This is called differently from the safepoint cleanup tasls, but could you call it trigger_cleanup_if_needed() instead?  Then I know it does the same/similar thing as the others without looking.
> Renamed to trigger_cleanup_if_needed().
>
> Also test_and_clear_cleanup_request() => has_cleanup_work_and_reset().
> Thomas asked for "has_work", to be consistent with
> String/Symbol/ResolvedMethodTable, but I think that's too generic here;
> what kind of work? (In the case of the tables, it's not always
> "cleanup" work.) Coleen suggested the "and_reset" suffix, to follow an
> existing convention.
>
> I also made a few corresponding internal name changes.
>
>> The implementation looks good.  I think it's good that you don't have the safepoint cleanup task timer around this.
> I added a comment about the lack of task timing, so it's clearly
> intentional and not simply forgotten.
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8226366/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8226366/open.01.inc/
>
> Testing: Local build and hotspot_tier1.
>

Nice!
Thanks,
Coleen