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

[Bertrand Delsart]

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

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