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

[David Holmes]

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