RFR: 8243996: Remove hardcoded field offsets from HotSpot

[David Holmes]

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
>