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

[coleen.phillimore at oracle.com]

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
>>