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

[Thomas Schatzl]

Hi,

On Mon, 2019-07-01 at 17:36 -0400, 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?
> 
> 
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 }
> 

Similar in serviceThread.cpp:136, it would be nice if the method were
named "has_work()" like others instead of
"test_and_clear_cleanup_request()". While the latter is technically
better, it raises the question whether it is the correct thing to do
here in some way when compared to others. Feel free to ignore this
comment though.

I *think* otherwise it is good, but I am kind of new to the OopStorage
stuff.

Thanks,
  Thomas