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

[Erik Osterlund]

Hi Kim,

On 2 Aug 2019, at 22:01, Kim Barrett <kim.barrett at oracle.com> wrote:

>> On Aug 2, 2019, at 9:29 AM, Erik Österlund <erik.osterlund at oracle.com> 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.
>> 
>> 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?
> 
> I prefer code that doesn't invoke undefined behavior, even if it
> might work (or even probably works) in practice.

Me too, unless it’s aliasing rules, which I don’t mind violating given the chance. Mind elaborating what you think I missed?

> And an iterator whose initial "current" is invalid?  You think
> *macros* are confusing?

That’s Java style iterators I guess. You call next until you can’t. Before you called next the first time, there is no current element that has been handed out. I don’t mind changing the iterator style though if you think that is confusing. It is my preferred style for simple linear iteration.

> Separating the oopstorages from the serial weak phases is interesting.
> I think I like that idea.

Glad you liked that part!

Thanks,
/Erik