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

[Coleen Phillimore]

On 9/9/15 3:55 PM, Ioi Lam wrote:
>
>
> On 9/9/15 11:36 AM, 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.
>>
>>
>>> 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".
>>
>>
>>> 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?
>
> I just checked, there's no serviceability agent code that uses 
> intrinsic_id. So we're safe. No work to be done there :-)
>
> ./src/share/vm/runtime/vmStructs.cpp:  nonstatic_field(Method, 
> _intrinsic_id, u1)
>
> ~/jdk/jdk9_rt/hotspot/agent$ find . -name \*.java | xargs grep 
> intrinsic_id | wc

Thanks Ioi, I don't see it either.   Sometimes the name varies in the SA 
(different format/case, etc).

So I think the only thing that needs to be done is run this through some 
more testing.  I've looked at the change and it looks fine otherwise.

And please file the bug to fix the footprint on 32 bit.  This is 
important for those platforms.

Thanks,
Coleen

> 0       0       0
>
> Thanks
> - Ioi
>
>
>> 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
>>>>
>>
>