RFR (M) 8135085: Change Method::_intrinsic_id from u1 to u2

[Ioi Lam]

On 9/9/15 12:00 PM, Coleen Phillimore wrote:
>
>
> On 9/9/15 2:36 PM, Aleksey Shipilev wrote:
>> Hi Coleen,
>>
>> On 09/09/2015 08:37 PM, Coleen Phillimore wrote:
>>> Increasing the size of this field unfortunately gives us a pretty large
>>> alignment gap in Method* which will affect footprint.
>> Where does this come from? As I mentioned in the original note, on my
>> Linux x86_64/gcc, sizeof(Method) is 88 bytes for both baseline and
>> patched version -- it would seem the alignment gaps are already present
>> where we can fit the u1 -> u2 expansion. It only expands to 96 bytes
>> when I add three additional u1 fields.
>
> You are right for 64 bits because AccessFlags is jint.   For 32 bits, 
> this will add a 3 byte alignment gap though.
>
>>
>>
>>> The good news is that the field _method_size is not actually needed.
>>> In order to increase intrinsic_id, you should also remove this field at
>>> the same time.   Then we'll only have a u1 alignment gap which we can
>>> use for something in the future.
>> Um. There is a Method::method_size() getter. All right, the only use of
>> it is Method::size(). Method::size() is used at least in
>> MetadataFactory::free_metadata()... I'm puzzled what you mean by
>> "_method_size is not actually needed".
>
> method_size() can be recoded as pseudo code: sizeof(Method)/wordSize + 
> is_native() ? 2 : 0;   The field is leftover when there were bytecodes 
> in Method*.
>>
>>
>>> Also, because you changed the vmStructs, there's likely a Java
>>> duplicated code change to make in the serviceability agent. There
>>> should be a basic test in the jtreg hotspot/test/serviceability/sa.
>>> You should run more tests on a change like this.   I will send you
>>> directions on how to run RBT internally.
>> Of course, but can anyone from runtime team take over? This is a core
>> runtime issue that affects everyone generally, and it is important for
>> an ongoing JDK project. Please help?
>
> It's pretty straightforward.  It doesn't seem like there's a lot more 
> work to finish it.   We could file a separate RFE to remove 
> _method_size if that will help.

This change looks good to me too, especially there is no footprint 
regression on 64-bit.

I'll file a separate RFE to remove _method_size.

Thanks
- Ioi
> Coleen
>
>>
>> Thanks,
>> -Aleksey
>>
>>> thanks,
>>> Coleen
>>>
>>> On 9/9/15 1:22 PM, Aleksey Shipilev wrote:
>>>> Hi,
>>>>
>>>> In VarHandles development, we have reached the tipping point when
>>>> Method::_intrinsic_id is not enough to be u1, as we have >255 
>>>> intrinsics:
>>>>     https://bugs.openjdk.java.net/browse/JDK-8135085
>>>>
>>>> This is a patch that changes u1 -> u2:
>>>>     http://cr.openjdk.java.net/~shade/8135085/webrev.00/
>>>>
>>>> Quite a few places in VM assume the _instance_id size, I changed those
>>>> as well, and also put more asserts where we assume the particular 
>>>> value
>>>> width.
>>>>
>>>> Testing:
>>>>     * JPRT on all platforms
>>>>     * Method::Method instrumentation to figure out the number of 
>>>> Methods
>>>> and their sizeof-s. There seems to be no regression in Method instance
>>>> size at least on Linux x86_64, because the instance alignment seems to
>>>> have enough gaps to fit the larger u2 field. (I played more, and it
>>>> seems three more u1 fields can be fitted in).
>>>>
>>>> Please review! If there are any problems/concerns, could anyone from
>>>> runtime team please take over this work? We will use this prototype
>>>> version in VarHandles to unblock the development for the time being.
>>>>
>>>> Thanks,
>>>> -Aleksey
>>>>
>>
>