RFR [S] 8087133: Improve sharing of native wrappers in SignatureHandlerLibrary

Coleen Phillimore coleen.phillimore at oracle.com
Mon Jun 22 19:44:27 UTC 2015


This change looks fine to me.
Coleen


On 6/17/15 12:14 AM, David Holmes wrote:
> On 17/06/2015 2:54 AM, Bertrand Delsart wrote:
>> On 15/06/2015 05:52, David Holmes wrote:
>>> On 12/06/2015 7:49 PM, Bertrand Delsart wrote:
>>>> On 12/06/2015 10:57, David Holmes wrote:
>>>>> Hi Bertrand,
>>>>>
>>>>> On 12/06/2015 6:12 PM, Bertrand Delsart wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This CR allows to reduce the number of generated fast native 
>>>>>> signature
>>>>>> handlers, based on a CPU-dependent normalization of the signatures.
>>>>>>
>>>>>> This will be pushed through hs-rt as part of a series of webrevs 
>>>>>> that
>>>>>> implement a closed JEP which adds support for some pregenerated 
>>>>>> code,
>>>>>> including these handlers.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8087133
>>>>>>
>>>>>> http://cr.openjdk.java.net/~bdelsart/8087133/webrev.01/webrev/index.html 
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> src/share/vm/runtime/signature.hpp
>>>>>
>>>>> In is_static:
>>>>>
>>>>> 120     return fingerprint & 1;
>>>>>
>>>>> should the 1 be a constant/enum? eg static_feature_size ?
>>>>
>>>> It would have to be a mask, not a size, but the mask is so simple that
>>>> static_feature_mask was not defined.
>>>>
>>>> Note that the "1" corresponds the setting in
>>>> Fingertprinter::fingerprint() in the same file:
>>>>      if (mh->is_static())  _fingerprint |= 1;
>>>>
>>>> I fully agree that enums might be worth it if used elsewhere but I in
>>>> fact added the is_static method here, next to the definition of
>>>> Fingerprinter", to avoid hard-coding this "& 1" elsewhere, hopefully
>>>> improving robustness against changes in the encoding :-)
>>>>
>>>> Would you still prefer me to define explicitly static_feature_mask and
>>>> use it, leading to: ((fingerprint & static_feature_mask) == 1) ?
>>>
>>> It's better not to have "magic numbers" but then that also argues for
>>> the 0 and -1 cases being covered by constants too. I'd at least like to
>>> see the 1 as a mask. Thanks.
>>
>> Thinking further, I instead defined 'is_static_bit'.
>>
>> New webrev:
>>
>> http://cr.openjdk.java.net/~bdelsart/8087133/webrev.02/webrev/index.html
>>
>> Incremental diff for webrev.01 to webrev.02:
>>
>> +++ hotspot/src/share/vm/runtime/signature.hpp
>> @@ -63,6 +63,8 @@
>>     // Fingerprinter.
>>     enum {
>>       static_feature_size    = 1,
>> +      is_static_bit        = 1,
>
> Indent needs fixing (two extra spaces) but otherwise looks good - no 
> need to re-review.
>
> Thanks,
> David
>
>> +
>>       result_feature_size    = 4,
>>       result_feature_mask    = 0xF,
>>       parameter_feature_size = 4,
>> @@ -117,7 +119,7 @@
>>
>>     static bool is_static(uint64_t fingerprint) {
>>       assert(fingerprint != (uint64_t)CONST64(-1), "invalid 
>> fingerprint");
>> -    return fingerprint & 1;
>> +    return fingerprint & is_static_bit;
>>     }
>>     static BasicType return_type(uint64_t fingerprint) {
>>       assert(fingerprint != (uint64_t)CONST64(-1), "invalid 
>> fingerprint");
>>
>> Regards,
>>
>> Bertrand.
>>
>>>
>>> David
>>> -----
>>>
>>>>>
>>>>> src/share/vm/interpreter/interpreterRuntime.cpp
>>>>>
>>>>>   tty->print_cr(" associated result handler: " PTR_FORMAT, rh_begin);
>>>>>
>>>>> You might find that needs p2i(rh_begin) on some platforms. I've been
>>>>> seeing weird inconsistencies in some code I've been working on.
>>>>
>>>> Will do. It passes JPRT and builds on my additional exotic 
>>>> platforms but
>>>> using p2i should be harmless and may indeed be safer.
>>>>
>>>> Thanks,
>>>>
>>>> Bertrand.
>>>>
>>>>>
>>>>> Otherwise seems okay.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>
>>>>>> In the general use case, this only slightly reduces the number of
>>>>>> generated native handlers, leading to a negligible code size gain.
>>>>>> However, this sharing is more critical for our closed pregenerated
>>>>>> code
>>>>>> JEP. This is why the CPU-dependent normalization, which triggers the
>>>>>> sharing, is currently only implemented on these closed platforms.
>>>>>>
>>>>>> The webrev also fixes a minor debug issue that was discovered while
>>>>>> validating the change on our closed platforms.
>>>>>> -XX:+PrintSignatureHandlers did not work correctly because
>>>>>> Interpreter::result_handler(return_type) is not always executable
>>>>>> code.
>>>>>> On some platforms, this is just a value used by the native wrappers
>>>>>> instead of code jumped to.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Bertrand
>>>>
>>>>
>>
>>



More information about the hotspot-runtime-dev mailing list