RFR: 8264016: [JVMCI] add some thread local fields for use by JVMCI

[David Holmes]

Hi Tom,

On 24/03/2021 4:06 pm, Tom Rodriguez wrote:
> On Tue, 23 Mar 2021 06:28:55 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> 8264016: [JVMCI] add some thread local fields for use by JVMCI
>>
>> Hi Tom,
>>
>> Is it feasible to create a JVMCI helper side-object that is only created when needed, rather than embedding all the fields directly in the JavaThread instance?
>>
>> Thanks,
>> David
> 
> Well the goal is to have storage that is only one or two loads away so adding a level of indirection isn't great.  Many of the other JVMCI fields aren't really performance critical so they could be an extra indirection away but that's not a compatible JVMCI change since they are directly written by Graal for deopt.  It also wouldn't save much space.  I know there is sensitivity around making JavaThread larger so I tried not to go too crazy.  Obviously I'd prefer to stick with what I have.

Understood.

> Out of curiosity I'd used the clang option ``-Xclang -fdump-record-layouts`` to look at JavaThread in 8, 11 and 17.  It's definitely getting fatter.  8 is 1024, 11 is 1264 and 17 is 1408, plus the huge alignment wastage caused by biased locking, though I guess that's not a problem in 17.  Anyway, I could probably get back the space I'm using by rearranging some of the fields of JavaThread.  There's a lot of wastage from switching between pointer, int and bool.  I haven't done a deep analysis of how much could be recovered but I could look into reducing the overall size JavaThread by repacking if it makes adding these fields more palatable.

Thanks for that info - that's quite a large growth since 11. And 
Biased-locking is still in 17 (removal deferred to 18) so we're still 
paying for additional alignment there.

Does the C++ compiler attempt to do any field packing or is everything 
laid out as we write it? I could see a couple of int fields in Thread 
that might trigger gaps, and then in JavaThread there is a very suspect 
layout here:

   // suspend/resume support
   volatile bool         _suspend_equivalent;     // Suspend equivalent 
condition
   jint                  _in_deopt_handler;       // count of deoptimization
                                                  // handlers thread is in
   volatile bool         _doing_unsafe_access;    // Thread may fault 
due to unsafe access
   bool                  _do_not_unlock_if_synchronized;

I don't know how frugal the C++ compiler actually is with bools, but 
perhaps some simple reordering there might reclaim some space?

Obviously I'd prefer not to have Thread/JavaThread keep growing, but 
just because you're the most recent requestor doesn't mean the burden 
should fall to you to fix the overall layout problem. So I'm okay with 
what you propose and will file a RFE to have someone look into 
optimising the layout to see if we can recoup some space.

>> src/hotspot/share/runtime/thread.hpp line 1020:
>>
>>> 1018:   intptr_t*  _jvmci_reserved0;
>>> 1019:   intptr_t*  _jvmci_reserved1;
>>> 1020:   oop        _jvmci_reserved_oop0;
>>
>> Can this use OopStorage? We've been getting rid of oop fields and the corresponding oops_do support.
> 
> Wouldn't using OopStorage require an extra level of indirection for the field?

Possibly - but there has been a very strong move to using oopStorage in 
any case. Probably best to ask Erik O./Kim/Coleen about that.

Thanks,
David
-----

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/3147
>