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

[Erik Österlund]

Hi Kim,

I had a look at the patch. Personally I'm not a big fan of using macro 
tricks. Perhaps because it's in caps, which makes me read the code in my 
head with a loud screaming voice, with a slightly angry italian accent. 
And I don't even know italian - it is crazy.

Anyway, I have come up with an alternative solution that is worth 
considering, I think, due to its simplicity.

Step 1: Move all oop storages to a new file, and add a utility for 
iterating over them:

webrev: http://cr.openjdk.java.net/~eosterlund/8227054/webrev.00/

This is quite straight forward. A new OopStorageSet AllStatic class has 
all oop storages, and offers some iterators.

Step 2: Change weak processor to automatically process oop storages:

webrev:http://cr.openjdk.java.net/~eosterlund/8227054/webrev.01/
incremental: http://cr.openjdk.java.net/~eosterlund/8227054/webrev.00_01/

I noticed that the weak processor has an enum with all phases, but 
pretty much always dealt with oop storage phases differently, with 
various checks if it is an oop storage phase, do something different, or 
assert that things are only done in an oop storage phase.
So I made processing of oop storages use the new iterators instead, and 
opt out from the enum macro dance.

To add a new OopStorage, you add it to the OopStorageSet by plugging in 
a declaration in OopStorageSet::StrongOopStorageSet or 
OopStorageSet::WeakOopStorageSet depending on strength, stick in a 
getter if you need direct access, and create an instance in 
OopStorageSet::initialize() - 3 lines in total. This is 2 lines less 
than your approach, but I think that is okay for the added simplicity.

In total, this patch removes more code than it introduces.

Thoughts?

Thanks,
/Erik


On 2019-07-26 22:39, Kim Barrett wrote:
>> On Jul 26, 2019, at 10:38 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> I like this change!
>>
>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp.udiff.html
>>
>> Can you remove some #include directives from the GC code since the oop storages are coming from oopStorages?
>>
>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/prims/resolvedMethodTable.cpp.udiff.html
>>
>> Does this still need to #include oopStorage.inline.hpp ?
>>
>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/prims/resolvedMethodTable.hpp.frames.html
>>
>> This doesn't seem to need to include oopStorage.hpp.  Might be others too.
> Thanks for suggesting some #include trimming.  I did some cleanup.
>
>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/gc/shared/oopStorages.hpp.html
>>
>> I thought our compilers were tolerant of a trailing comma in enumerations?
> Solaris Studio doesn't like trailing commas in enums.
>
>> The macros aren't bad though.  It seems like it would be easy to add a new OopStorage if we wanted to, but it would be better to use an existing vm weak or vm strong oop storage, if we wanted to move more oops into an oop storage (say for JFR).
> Right.  Also easy to remove one.  (Someone told me there are ideas
> afloat that might remove the need for the ResolvedMethodTableWeak
> storage object.)
>
> I don't expect the set to be changing frequently.  But it's a bit of a
> pain to track down all the right places.  (Even still, because the GCs
> still have explicit knowledge of the full set, though this change
> provides tools to address that.)
>
>> This change looks great apart from trying to remove more #includes.
> Thanks.
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8227054/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8227054/open.01.inc/
>
>