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

[Erik Österlund]

Hi Kim,

On 2019-08-12 05:26, Kim Barrett wrote:
>> On Aug 10, 2019, at 3:52 AM, Erik Osterlund <erik.osterlund at oracle.com> wrote:
>>
>> 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?
> 
> Just to be clear, there are no x-macros here, just a couple very
> simple macros.  Using x-macros would have reduced those blocks of
> macro invocations to one-liners.

Okay.

> Can you describe the IDE confusion?  I can think of lots of potential
> issues, depending on how smart/not-so-smart a given IDE might be, but
> I don't know what actual problems you might be worried about.

The two main ones I am concerned about is go-to-definition. There is no 
real definition now, so you will get to a macro instead. The other 
concern is the inability to list all callers of the getter. Since there 
is no definition outside of macro land, you can't do that, and will have 
to resort to raw text search instead.

I for example use the rtags plugin to Emacs to find my way around 
hotspot using such functionality. But obviously there are lots of other 
tools doing similar things.

I usually try not to break the ability to navigate the code by having it 
generated by macros, unless it's somehow painful not to do it with 
macros, keeping that as a plan B. But here it looks trivial, and it 
doesn't look like the use of macros buys us anything more than another 
mental indirection to look through.

> For me, the macros improve readability by emphasizing the parts that
> are different (the interesting parts) and deemphasizing the parts that
> are the same (the boring parts).

For me it's the opposite. :/
If I want to know the name of e.g. the JNI weak getter, I have to first 
read the OOPSTORAGE_ACCESSOR(jni_weak) line, then go to the definition 
of OOPSTORAGE_ACCESSOR, and read the macro to figure out how that 
invocation will generate the getter, instead of just reading the 
one-liner getter, like this:
static OopStorage* jni_weak() { return storage(jni_weak_index); }

> I looked at the logging of OopStorage processing; the use of the
> stringized identifiers rather than bespoke strings looked fine to me.
> Indeed, it has the small additional benefit for someone who is a
> developer that the name exactly matches the associated getter. (Of
> course, that doesn't matter to a non-developer log user.)

Okay. I think it's nicer for users to get a pretty name instead of an 
identifier in the code.

>> Apart from those two things, I really liked the approach!
> 
> Thanks.  Hopefully we’re approaching a conclusion on this change.

I hope so.

Thanks,
/Erik