RFR: 8243996: Remove hardcoded field offsets from HotSpot

[coleen.phillimore at oracle.com]

On 5/14/20 5:41 AM, Erik Österlund wrote:
> Hi Coleen,
>
> Thanks for the review.
>
> On 2020-05-13 20:07, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 5/13/20 12:00 PM, John Rose wrote:
>>> I like this cleanup!
>>>
>>> There are new tricky dependencies between the interpreter and other
>>> runtime bits, and class loading; they are adequately documented in new
>>> comments.
>>>
>>> A few comments of my own:
>>>
>>> The whitespace on the new line in vmSymbols.hpp needs to be adjusted to
>>> resemble whitespace on neighboring lines.  Hotspot is a jungle of 
>>> old code,
>>> and our new code needs to blend into that jungle, not make spots of new
>>> color (unless of course there is a purpose to some new color).
>>>
>>> I don’t want to gripe needlessly about identifier choice, but… maybe I
>>> do in this case:  “interpreter_init_code” and 
>>> “interpreter_init_code_stub”
>>> seem overly cryptic to me; they don’t help me remember what they do.
>>> I suggest that“interpreter_init_stubs” and “interpreter_init_code” or
>>> “interpreter_init_stub_code” and “interpreter_init_main_code” would
>>> be easier on maintainers.  But if it’s just me, leave it as is.  We 
>>> coders eat
>>> cryptic identifiers for breakfast.
>>
>> Erik and I came up with these names earlier today as they were more 
>> descriptive than some original names. "interpreter_init_stubs" and 
>> "interpreter_init_code" seem okay also.
>>>
>>> Can someone remind me (a) which platforms use the C++ interpreter,
>>> and (b) how well they are tested?
>>>
>>> I spot-checked for queries to the hard-coded offsets, and found enough
>>> “guarantee(this_offset != 0…)” to make me feel secure. If there’s a bug
>>> related to this change, it’s quite possibly a zero offset being used 
>>> on some
>>> path in the JVM which is rarely executed.  (Such a bug could be 
>>> introduced
>>> in the future, right?)  Usually we avoid such bugs by encapsulating 
>>> variable
>>> state into C++ access functions which perform their own assertions, 
>>> rather
>>> than requiring users of the variable to make their own checks.
>>>
>>> Did you look into making all those public variables private, and 
>>> wrapping
>>> access functions around them?  I think that would make for a more 
>>> complete
>>> cleanup.  It would touch more places in the interpreter, including 
>>> platform
>>> code, which I think would be a good thing.  It would (a) allow 
>>> review now
>>> of dependency checks in the interpreter, and (b) force downstream 
>>> porters
>>> to similarly review their use of hard-coded offsets.
>>
>> This seems like a good suggestion for these classes.  For the most 
>> part, javaClasses foo_offset are private.
>
> I'm trying to move the pre-existing various offset style 
> inconsistencies of JavaClasses
> (50-50 use of leading '_', sometimes using accessors, sometimes being 
> public, sometimes
> asserting offsets are initialized, etc) into a new RFR. I think there 
> is plenty of weirdness there
> and don't think fixing it belongs to this change.

Oh I thought the additional change should only apply to the fields of 
java_lang_ref_Reference class which are public, but I'm fine with a 
future RFE.

Coleen
>
>> I was going to reply to this thread "Looks good to me" and thanks for 
>> adding the comments.
>
> Thanks!
>
> /Erik
>
>> Coleen
>>>
>>> — John
>>>
>>>> On May 13, 2020, at 8:26 AM, Erik Österlund 
>>>> <erik.osterlund at oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Let's remove hardcoded field offsets. Main trick to get this done 
>>>> is to split interpreter initialization
>>>> into two parts: 1. reserve the interpreter memory, 2. generate the 
>>>> interpreter. This allows reserving
>>>> it before loading methods (which generates i2c adapters that assert 
>>>> things are "in the interpreter",
>>>> requiring knowledge about its address range), and then load the 
>>>> methods, and then generate the interpreter.
>>>> This moving of interpreter code generation until after methods have 
>>>> been loaded allows offsets to be
>>>> looked up, needed by code generation. That way, we no longer need 
>>>> to hardcode field offsets, which
>>>> makes it less annoying to change the field layout code in the future.
>>>>
>>>> Thanks to Coleen for pre-reviewing this.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8243996
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8243996/webrev.01/
>>>>
>>>> Thanks,
>>>> /Erik
>>
>