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

[Erik Osterlund]

Hi Kim,

Thank you for taking this for another spin. I like the new version a lot better. I think you convinced me on using the private enum being a better idea than my reinterpret_cast.

I only have two questions left:

1) How would you feel about not using the macro for generating the getters? It looks like 1 short line for the getter vs 1 short line for the macro. In other words, not much typing is saved using the X macro here. But it will confuse IDEs. So I would have preferred the explicit getters.

2) Similar thought for the initialization of OopStorages where the name is generated from the macro. I thought the name was meant to be in more human readable form as we print them out in logs. So again, I would have loved explicit initialization, which is still just 1 trivial line per OopStorage.

If you have strong opinions about keeping those macros, I will not oppose it. But maybe you agree that retaining that IDE awareness of the code and nice printouts in the logs is worth doing those one liners without X macros?

Apart from those two things, I really liked the approach!

Thanks,
/Erik

On 10 Aug 2019, at 00:18, Kim Barrett <kim.barrett at oracle.com> wrote:

>> On Aug 5, 2019, at 9:48 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>> 
>> Hi Coleen,
>> 
>>> On 2019-08-05 15:23, coleen.phillimore at oracle.com wrote:
>>> I look forward to seeing Kim's next version.
>> 
>> Me too!
> 
> Here is a new revision, taking into account the feedback that fewer
> macros (and in particular no x-macros) would be appreciated.
> 
> OopStorageSet (was OopStorages) now uses an internal enum (no longer
> public) to define indexes and ranges of indexes into the global array
> of OopStorage objects.
> 
> WeakProcessorPhases no longer has named enumerators for OopStorage
> phases, though the range of values is still allocated.  Various other
> changes in WeakProcessor and friends to account for and make use of
> OopStorageSet. 
> 
> I've also taken Erik's suggestion of using iterator objects to package
> up the iteration, rather than using FOR_EACH_xxx macros.  That also
> got applied to the WeakProcessorPhases.
> 
> For extra credit (or perhaps a bridge too far), the new iterators
> provide begin/end functions, so they support range-based-for as soon
> as it becomes available, allowing some syntactic improvements.
> 
> Webrev (no incremental, the changes from the previous version are
> pretty substantial and not really worth comparing):
> http://cr.openjdk.java.net/~kbarrett/8227054/open.02/
> 
> Testing:
> mach5 tier1-3
>