RFR: 8243996: Remove hardcoded field offsets from HotSpot

[Erik Österlund]

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