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

[coleen.phillimore at oracle.com]

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?

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.

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.

818 void OopStorage::request_cleanup_if_needed() {
819 MonitorLocker ml(Service_lock, Monitor::_no_safepoint_check_flag);
820 if (Atomic::load(&needs_cleanup_requested) &&
821 !needs_cleanup_notified &&
822 (os::javaTimeNanos() > cleanup_permit_time)) {
823 needs_cleanup_notified = true;
824 ml.notify_all();
825 }
826 }


The implementation looks good.  I think it's good that you don't have 
the safepoint cleanup task timer around this.

Thanks,
Coleen

On 6/25/19 10:38 PM, Kim Barrett wrote:
> Please review this change to OopStorage's notifications to the ServiceThread
> to perform empty block deletion.  The existing mechanism (introduced by
> JDK-8210986) is driven by entry allocation, and may arbitrarily delay such
> cleanup, or alternatively may be much too enthusiastic about waking up the
> ServiceThread.
>
> The new mechanism does not depend on allocations.  Instead, a new safepoint
> cleanup task is used to (irregularly) check for pending requests and notify
> the ServiceThread.  That notification has a time-based throttle, and also
> avoids duplicate notifications.  Also, requests are now only recorded for
> to-empty transitions and not for full to not-full transitions.
>
> Changed the work limit for delete_empty_blocks to have a small surplus to
> avoid some common cases with small number of blocks leading to unnecessarily
> spinning the ServiceThread.
>
> While making these changes, noticed and fixed a problem in block allocation
> that could result in a mistaken report of allocation failure.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8226366
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8226366/open.00/
>
> Testing:
> mach5 tier1-5
>
> Locally ran gc/stress/TestReclaimStringsLeaksMemory.java with some extra
> logging and verified that the number of ServiceThread notifications was
> reduced by a *lot*, down to something reasonable.
>