RFR: 8210986: Add OopStorage cleanup to ServiceThread

[Kim Barrett]

> On Oct 25, 2018, at 7:00 PM, coleen.phillimore at oracle.com wrote:
> On 10/25/18 6:26 PM, Kim Barrett wrote:
>>> On Oct 25, 2018, at 6:01 PM, coleen.phillimore at oracle.com wrote:
>>> 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.
> 
> The style guide says that we avoid of noise word get for cases like this:
> 
> Block* block() const { return _block; }
> void set_block(Block* b) { _block = b; }
> 
> If the function is doing anything else, you can say get.  Also because 'block' is a verb, it makes it confusing.

Sorry, but I don't agree.  
 
That's a very narrow interpretation of "getter", and suggests the
naming convention depends on the underlying implementation.  But a
primary purpose of providing a function-based API is information
hiding; the implementation can be changed without affecting clients.
Tying the name to the implementation as suggested is contrary to that
purpose.  So I think that interpretation is incorrect.

As to the potential noun/verb confusion for "block", this function
appears in a context where objects of type "Block" are being used, and
it returns such an object.  It seems to me that ought to be sufficient
disambiguation.

>>> 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?
>> […]  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.
> 
> Oh well, I see why it can't be static.  Maybe somewhere better will occur to us when we add more.  This looks okay to me.

Darn.  I was really hoping a better location would come up.