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

[Bertrand Delsart]

Thanks Coleen,

Bertrand.

On 22/06/2015 21:44, Coleen Phillimore wrote:
>
> 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
>>>>>
>>>>>
>>>
>>>
>


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38330 Montbonnot Saint Martin,                             FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~