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

[Ioi Lam]

On 9/9/15 1:24 PM, Coleen Phillimore wrote:
>
>
> 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.
>

I filed https://bugs.openjdk.java.net/browse/JDK-8135284 " Remove 
Method::_method_size field"

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