RFR: 8227054: ServiceThread needs to know about all OopStorage objects

[David Holmes]

On 1/08/2019 8:03 am, coleen.phillimore at oracle.com wrote:
> oopStorage.hpp has different things in it.  oopStorageCollection maybe?  I don't like any of these other names.  I don't like this name either. 

OopStorageSet is better (and shorter) than OopStorageCollection.

They are all better IMO than OopStorages.

But you need a second reviewer for this anyway, so lets get some 
additional input from them.

David
-----

> 
> On 7/31/19 2:13 AM, David Holmes wrote:
>> Hi Coleen,
>>
>> I've sat on this for a few hours :)
>>
>> On 31/07/2019 8:15 am, coleen.phillimore at oracle.com wrote:
>>> On 7/30/19 5:11 PM, David Holmes wrote:
>>>> On 31/07/2019 6:59 am, Kim Barrett wrote:
>>>>>> On Jul 29, 2019, at 10:27 PM, David Holmes 
>>>>>> <david.holmes at oracle.com> wrote:
>>>>>>
>>>>>> Hi Kim,
>>>>>>
>>>>>> A meta-comment: "storages" is not a well formed term. Can we have 
>>>>>> something clearer, perhaps OopStorageManager, or something like that?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>
>>>>> Coleen suggested the name OopStorages, as the plural of OopStorage.
>>>>
>>>> "storage" doesn't really have a plural in common use.
>>>
>>> Well this isn't common use.  There are more than one oopStorage 
>>> things in oopStorages.
>>>>
>>>>> (Unpublished versions of the change had a different name that I didn't
>>>>> really like and Coleen actively disliked.)  Coleen and I both have an
>>>>> antipathy toward "Manager" suffixed names, and I don't see how it's
>>>>> any clearer in this case.  "Set" suggests a wider API.
>>>>>
>>>>> Also, drive-by name bikeshedding doesn't carry much weight.
>>>>
>>>> Okay how about its really poor form to have classes and files that 
>>>> differ by only one letter. I looked at this to see what it was about 
>>>> and had to keep double-checking if I was looking at OopStorage or 
>>>> OopStorages. In addition OopStorages conveys no semantic meaning to me.
>>>>
>>>
>>> This might be confusing to someone who doesn't normally look at the 
>>> code. 
>>
>> The fact they differ by only one letter leads to an easy source of 
>> mistakes in both reading and writing the code. The very first change I 
>> saw in the webrev was:
>>
>>   #include "gc/shared/oopStorage.inline.hpp"
>> + #include "gc/shared/oopStorages.hpp"
>>
>> and I immediately thought it was a mistake because the .hpp would be 
>> included by the .inline.hpp file - but I'd missed the 's'.
> 
> I was going to say that ideally the runtime code only needs 
> oopStorages.hpp, and not the details of oopStorage.inline.hpp (except 
> WeakProcessor) but there are some other cleanups that should happen first.
> 
>>
>>> If you come up with a better name than Manager, it might be okay to 
>>> change.  So far, our other name ideas weren't better than just the 
>>> succinct "Storages". Meaning multiple oopStorage objects (they're not 
>>> objects, that's a bad name because it could be confusing with oops 
>>> which are also called objects).
>>
>> OopStorageUnit
>> OopStorageDepot
>> OopStorageFactory
>> OopStorageHolder
>> OopStorageSet
>>
>> Arguably this could/should be folded into OopStorage itself and avoid 
>> the naming issues altogether.
> 
> oopStorage.hpp has different things in it.  oopStorageCollection maybe? 
> I don't like any of these other names.  I don't like this name either.
> 
> Coleen
> 
>>
>> Cheers,
>> David
>>
>> P.S. What's so bad about Manager? :)
>>
>>> Coleen
>>>
>>>> Thanks,
>>>> David
>>>
>