RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

dean.long
https://bugs.openjdk.java.net/browse/JDK-8145579
http://cr.openjdk.java.net/~dlong/8145579/webrev/

This change fixes certain non-trivial methods being marked as trivial.  A method should only be marked as trivial if C2 cannot generate better code.  The two cases fixed here are calling intrinsics, and Object.<init>, which has a hidden call to register finalizers.  When run with  http://cr.openjdk.java.net/~shade/8145579/benchmarks.jar, the test methods now end up compiled at level 4 instead of level 1.

dl
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

Aleksey Shipilev-4
On 12/04/2017 06:44 PM, [hidden email] wrote:
> https://bugs.openjdk.java.net/browse/JDK-8145579
> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>
> This change fixes certain non-trivial methods being marked as trivial.  A method should only be
> marked as trivial if C2 cannot generate better code.  The two cases fixed here are calling
> intrinsics, and Object.<init>, which has a hidden call to register finalizers. 

I am good with the fix, but my issue is with the essence of that shortcut. We can keep piling on
exceptions on top of exceptions to that triviality rule, or we can just dispense with the idea that
we can guess what methods are trivial and/or remember to fix it up every time something changes.

Is it really worth falling back to C1 for trivial methods? I.e. is C2 compilation for those methods
too costly (which would seem counter-intuitive)? Can we remove that triviality check, and see if we
run fine without it?

Thanks,
-Aleksey



signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

dean.long
Hi Aleksey.  Thanks for the review,


On 12/4/17 12:10 PM, Aleksey Shipilev wrote:

> On 12/04/2017 06:44 PM, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8145579
>> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>>
>> This change fixes certain non-trivial methods being marked as trivial.  A method should only be
>> marked as trivial if C2 cannot generate better code.  The two cases fixed here are calling
>> intrinsics, and Object.<init>, which has a hidden call to register finalizers.
> I am good with the fix, but my issue is with the essence of that shortcut. We can keep piling on
> exceptions on top of exceptions to that triviality rule, or we can just dispense with the idea that
> we can guess what methods are trivial and/or remember to fix it up every time something changes.
>
> Is it really worth falling back to C1 for trivial methods? I.e. is C2 compilation for those methods
> too costly (which would seem counter-intuitive)? Can we remove that triviality check, and see if we
> run fine without it?

I agree, we should take another look at that code.  We have reopened
8076988 to look into that for 11.

dl

> Thanks,
> -Aleksey
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

Vladimir Kozlov
In reply to this post by dean.long
Good.

Thanks,
Vladimir

On 12/4/17 9:44 AM, [hidden email] wrote:

> https://bugs.openjdk.java.net/browse/JDK-8145579
> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>
> This change fixes certain non-trivial methods being marked as trivial.  
> A method should only be marked as trivial if C2 cannot generate better
> code.  The two cases fixed here are calling intrinsics, and
> Object.<init>, which has a hidden call to register finalizers.  When run
> with http://cr.openjdk.java.net/~shade/8145579/benchmarks.jar 
> <http://cr.openjdk.java.net/%7Eshade/8145579/benchmarks.jar>, the test
> methods now end up compiled at level 4 instead of level 1.
>
> dl
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

dean.long
Thanks Vladimir.

dl


On 12/4/17 2:11 PM, Vladimir Kozlov wrote:

> Good.
>
> Thanks,
> Vladimir
>
> On 12/4/17 9:44 AM, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8145579
>> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>>
>> This change fixes certain non-trivial methods being marked as
>> trivial.  A method should only be marked as trivial if C2 cannot
>> generate better code.  The two cases fixed here are calling
>> intrinsics, and Object.<init>, which has a hidden call to register
>> finalizers.  When run with
>> http://cr.openjdk.java.net/~shade/8145579/benchmarks.jar 
>> <http://cr.openjdk.java.net/%7Eshade/8145579/benchmarks.jar>, the
>> test methods now end up compiled at level 4 instead of level 1.
>>
>> dl

Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

Tobias Hartmann-2
In reply to this post by dean.long
Hi Dean,

this looks good to me!

Thanks,
Tobias

On 04.12.2017 18:44, [hidden email] wrote:

> https://bugs.openjdk.java.net/browse/JDK-8145579
> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>
> This change fixes certain non-trivial methods being marked as trivial.  A method should only be marked as trivial if C2
> cannot generate better code.  The two cases fixed here are calling intrinsics, and Object.<init>, which has a hidden
> call to register finalizers.  When run with  http://cr.openjdk.java.net/~shade/8145579/benchmarks.jar
> <http://cr.openjdk.java.net/%7Eshade/8145579/benchmarks.jar>, the test methods now end up compiled at level 4 instead of
> level 1.
>
> dl
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

Aleksey Shipilev-4
In reply to this post by dean.long
On 12/04/2017 10:07 PM, [hidden email] wrote:

> On 12/4/17 12:10 PM, Aleksey Shipilev wrote:
>> On 12/04/2017 06:44 PM, [hidden email] wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8145579
>>> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>>>
>>> This change fixes certain non-trivial methods being marked as trivial.  A method should only be
>>> marked as trivial if C2 cannot generate better code.  The two cases fixed here are calling
>>> intrinsics, and Object.<init>, which has a hidden call to register finalizers.
>> I am good with the fix, but my issue is with the essence of that shortcut. We can keep piling on
>> exceptions on top of exceptions to that triviality rule, or we can just dispense with the idea that
>> we can guess what methods are trivial and/or remember to fix it up every time something changes.
>>
>> Is it really worth falling back to C1 for trivial methods? I.e. is C2 compilation for those methods
>> too costly (which would seem counter-intuitive)? Can we remove that triviality check, and see if we
>> run fine without it?
>
> I agree, we should take another look at that code.  We have reopened 8076988 to look into that for 11.
Thanks, that makes sense.

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFR(XS) 8145579: SimpleThresholdPolicy assumes non-trivial methods to be trivial

dean.long
In reply to this post by Tobias Hartmann-2
Thanks Tobias!

dl


On 12/4/17 11:07 PM, Tobias Hartmann wrote:

> Hi Dean,
>
> this looks good to me!
>
> Thanks,
> Tobias
>
> On 04.12.2017 18:44, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8145579
>> http://cr.openjdk.java.net/~dlong/8145579/webrev/
>>
>> This change fixes certain non-trivial methods being marked as trivial.  A method should only be marked as trivial if C2
>> cannot generate better code.  The two cases fixed here are calling intrinsics, and Object.<init>, which has a hidden
>> call to register finalizers.  When run with  http://cr.openjdk.java.net/~shade/8145579/benchmarks.jar
>> <http://cr.openjdk.java.net/%7Eshade/8145579/benchmarks.jar>, the test methods now end up compiled at level 4 instead of
>> level 1.
>>
>> dl