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

[Per Liden]

Hi Erik & Kim,

On 8/2/19 3:29 PM, Erik Österlund wrote:
> 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.

I kind of had the same reaction. I stared at the initial patch for a 
good 30 minutes and then gave up. That x-macros makes this very 
difficult to read and follow, and it doesn't look like they actually 
solve anything important.

> 
> 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 a lot Erik, for taking the time to do this proposal. This looks a 
lot more straight forward and sane to me.

src/hotspot/share/runtime/jniHandles.cpp
----------------------------------------
  186 void JNIHandles::weak_oops_do(OopClosure* f) {
  187   OopStorageSet::weak_jni();
  188 }

Missing "->weak_oops_do(f)" there.

Other than than that, I'd be happy with this proposal.

cheers,
Per

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