RFR: 8243996: Remove hardcoded field offsets from HotSpot

[Erik Österlund]

Thanks David.

/Erik

> On 15 May 2020, at 00:49, David Holmes <david.holmes at oracle.com> wrote:
> 
> Nothing further from me.
> 
> Thanks,
> David
> 
>> On 14/05/2020 7:37 pm, Erik Österlund wrote:
>> Hi David,
>> Thanks for having a look at this!
>>> On 2020-05-14 07:19, David Holmes wrote:
>>> Hi Erik,
>>> 
>>> On 14/05/2020 1:26 am, Erik Österlund 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/
>>> 
>>> This seems like a good simplification, and allows for future flexibility. Initialization order changes are always tricky as the dependencies are not always obvious - there may be logging or error path related code that eventually assumes the interpreter has been generated - but we can deal with such issues if they arise.
>>> 
>>> Generally all seems fine to me - I won't comment on the colour of the bikeshed. :)
>> Phew! ;)
>>> One small comment:
>>> 
>>> src/hotspot/share/runtime/init.cpp
>>> 
>>> ! void interpreter_init_code();      // after methods loaded
>>> 
>>> given the later comment:
>>> 
>>> +   interpreter_init_code(); // after javaClasses_init and before any method gets linked (not loaded)
>>> 
>>> should the first comment be expanded to:
>>> 
>>> void interpreter_init_code();      // after methods loaded, but before they are linked
>>> 
>>> ?
>>> 
>>> Also IIUC the parenthetical "(not loaded)" alludes to the currently incorrect comment:
>>> 
>>> void invocationCounter_init(); // before any methods loaded
>>> 
>>> but once you've fixed that there's no context to know what the "(not loaded)" relates to so it can just be dropped.
>> I think I hopefully fixed the comments there the way you envision it in the latest webrev:
>> http://cr.openjdk.java.net/~eosterlund/8243996/webrev.02/
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8243996/webrev.01..02/
>> Although about loading vs linking it's not strictly true always. With CDS, the loading also links, but that linking is
>> a bit different and unproblematic for these initialization purposes. I tried to keep it simple enough to get the gist
>> of it in these one liner comments, instead of expanding the (for this context) unnecessarily complicated whole truth.
>> Hope that is okay.
>> Thanks,
>> /Erik
>>> Thanks,
>>> David
>>> -----
>>> 
>>>> Thanks,
>>>> /Erik