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

[David Holmes]

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.

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