RFR: 8210986: Add OopStorage cleanup to ServiceThread

[Kim Barrett]

> On Oct 25, 2018, at 6:01 PM, coleen.phillimore at oracle.com wrote:

Thanks for looking at this.

> http://cr.openjdk.java.net/~kbarrett/8210986/open.00/src/hotspot/share/gc/shared/oopStorage.cpp.frames.html
> 
> 392 // it is called in all kinds of contexts where even quote low ranked locks
> 
> typo: s/quote/quite/

Well spotted.  Will fix before pushing.

> 425 Block* block = block_for_allocation();
> 426 if (block == NULL) return NULL; // Block allocation failed.
> 
> This could be "get_block_for_allocation" because it's not blocking (afaict).  Name is somewhat ambiguous.

Hotspot style guide says getters are noun phrases, with no “get_" noise word.

> http://cr.openjdk.java.net/~kbarrett/8210986/open.00/src/hotspot/share/runtime/serviceThread.cpp.frames.html
> 
> ick.  Since this is a static list, can it be declared outside the scope of ServiceThread::entry(), like before the functions that loop through the oopstorages?
> 
> 109 OopStorage* const oopstorages[] = {
> 110 JNIHandles::global_handles(),
> 111 JNIHandles::weak_global_handles(),
> 112 StringTable::weak_storage(),
> 113 SystemDictionary::vm_weak_oop_storage()
> 114 };
> 115 const size_t oopstorage_count = ARRAY_SIZE(oopstorages);
> 116
> 
> I think it would bother me less that we have to have all the OopStorages listed somewhere, if it were static scope and not in the service thread function.  Are these all the ones we have?

It has to be at function scope.  If it were at file scope, it might
(and likely would) be initialized before the various OopStorage
objects have been initialized, and we'd have an array of NULLs.

I didn't declare it (function scope) static since it doesn't matter.
It's only constructed once on the single entry to the function.

And yes, this array is supposed to be complete.  If there were any
other places that needed a similarly complete set, I'd make a helper
function.  But so far, there isn't such a place, so I'm not sure where
I'd put such a function.  But I admit I'm not crazy about this array
being here.  It's not an obvious place to look if one were adding
another OopStorage.

> Otherwise I think this is fine.

Thanks.

>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8210986
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8210986/open.00/
>> 
>> Testing:
>> Mach5 tier1-5.
>> Ran some performance tests to verify no regressions due to additional
>> load on the Service thread.