[10] RFR(XS): 8169766: c1 + Xcomp reresolving call target every invocation

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] RFR(XS): 8169766: c1 + Xcomp reresolving call target every invocation

Nils Eliasson
Hi,

Please review this patch.

In bug https://bugs.openjdk.java.net/browse/JDK-8160543 I changed the
behaviour for some calls in c1 generated code to make it comply with the
VM-spec - LinkageError must be thrown before NPE.

One detail was missed in that fix: Some calls, that was static bound and
became optimized virtual calls, now became vanilla virtual calls. The
virtual calls use the unverified entry, but for the static_bound calls
the reciever_klass was not set making the receiver check fail on every
invocation.

This scenario is only common with Xcomp when methods are compiled that
have call targets that have not been run earlier. Also a c1 thing, c2
uses uncommontraps for this case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8160543

Webrev: http://cr.openjdk.java.net/~neliasso/8169766/webrev.01/

Regards,

Nils Eliasson

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR(XS): 8169766: c1 + Xcomp reresolving call target every invocation

Vladimir Kozlov
Good.

Question. Both, this and 8160543, bugs talk about C1 only. Does the
problem exist with C2 too? Comments in compiledIC.cpp also talk about
C1. Should we guard 8160543 and this changes in compiledIC.cpp with
if (method_code->is_compiled_by_c1()) ?

Thanks,
Vladimir

On 8/7/17 7:16 AM, Nils Eliasson wrote:

> Hi,
>
> Please review this patch.
>
> In bug https://bugs.openjdk.java.net/browse/JDK-8160543 I changed the
> behaviour for some calls in c1 generated code to make it comply with the
> VM-spec - LinkageError must be thrown before NPE.
>
> One detail was missed in that fix: Some calls, that was static bound and
> became optimized virtual calls, now became vanilla virtual calls. The
> virtual calls use the unverified entry, but for the static_bound calls
> the reciever_klass was not set making the receiver check fail on every
> invocation.
>
> This scenario is only common with Xcomp when methods are compiled that
> have call targets that have not been run earlier. Also a c1 thing, c2
> uses uncommontraps for this case.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8160543
>
> Webrev: http://cr.openjdk.java.net/~neliasso/8169766/webrev.01/
>
> Regards,
>
> Nils Eliasson
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR(XS): 8169766: c1 + Xcomp reresolving call target every invocation

Nils Eliasson

Hi Vladimir,


On 2017-08-07 18:42, Vladimir Kozlov wrote:
Good.

Question. Both, this and 8160543, bugs talk about C1 only. Does the problem exist with C2 too?

The problem only exists with C1. The code is shared with C2, but C2 won't be affected because it never tries to emit optimized calls to unloaded methods.

(Static bound methods that are loaded have the is_optimized flag set - so they still become optimized calls)

Comments in compiledIC.cpp also talk about C1. Should we guard 8160543 and this changes in compiledIC.cpp with
if (method_code->is_compiled_by_c1()) ?

No, I am removing the c1 specific parts, what's left is used by both.

The static_bound argument isn't used anymore and can be removed from the function call:

http://cr.openjdk.java.net/~neliasso/8169766/webrev.02/

If I put a diff of the two changes in this function together it becomes more clear why the first one was insufficient.


-    if (static_bound || is_optimized) {
+    if (is_optimized) {
       entry      = method_code->verified_entry_point();
     } else {
       entry      = method_code->entry_point();
     }


....

-    info.set_compiled_entry(entry, (static_bound || is_optimized) ? NULL : receiver_klass, is_optimized);
+    info.set_compiled_entry(entry, is_optimized                   ? NULL : receiver_klass, is_optimized);

Thanks for having a look,
Nils 

Thanks, Vladimir On 8/7/17 7:16 AM, Nils Eliasson wrote:
Hi, Please review this patch. In bug https://bugs.openjdk.java.net/browse/JDK-8160543 I changed the behaviour for some calls in c1 generated code to make it comply with the VM-spec - LinkageError must be thrown before NPE. One detail was missed in that fix: Some calls, that was static bound and became optimized virtual calls, now became vanilla virtual calls. The virtual calls use the unverified entry, but for the static_bound calls the reciever_klass was not set making the receiver check fail on every invocation. This scenario is only common with Xcomp when methods are compiled that have call targets that have not been run earlier. Also a c1 thing, c2 uses uncommontraps for this case. Bug: https://bugs.openjdk.java.net/browse/JDK-8160543 Webrev: http://cr.openjdk.java.net/~neliasso/8169766/webrev.01/ Regards, Nils Eliasson
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR(XS): 8169766: c1 + Xcomp reresolving call target every invocation

Vladimir Kozlov
On 8/11/17 7:42 AM, Nils Eliasson wrote:

> Hi Vladimir,
>
>
> On 2017-08-07 18:42, Vladimir Kozlov wrote:
>> Good.
>>
>> Question. Both, this and 8160543, bugs talk about C1 only. Does the
>> problem exist with C2 too?
>
> The problem only exists with C1. The code is shared with C2, but C2
> won't be affected because it never tries to emit optimized calls to
> unloaded methods.
>
> (Static bound methods that are loaded have the is_optimized flag set -
> so they still become optimized calls)
>
>> Comments in compiledIC.cpp also talk about C1. Should we guard 8160543
>> and this changes in compiledIC.cpp with
>> if (method_code->is_compiled_by_c1()) ?
>
> No, I am removing the c1 specific parts, what's left is used by both.
>
> The static_bound argument isn't used anymore and can be removed from the
> function call:
>
> http://cr.openjdk.java.net/~neliasso/8169766/webrev.02/

Good.

Thanks,
Vladimir

>
> If I put a diff of the two changes in this function together it becomes
> more clear why the first one was insufficient.
>
>
> - if (static_bound || is_optimized) {
> + if (is_optimized) {
> entry = method_code->verified_entry_point();
> } else {
> entry = method_code->entry_point(); }
> .... - info.set_compiled_entry(entry, (static_bound || is_optimized) ?
> NULL : receiver_klass, is_optimized);
> + info.set_compiled_entry(entry, is_optimized ? NULL : receiver_klass,
> is_optimized);
>
>
> Thanks for having a look,
> Nils
>
>> Thanks, Vladimir On 8/7/17 7:16 AM, Nils Eliasson wrote:
>>> Hi, Please review this patch. In bug
>>> https://bugs.openjdk.java.net/browse/JDK-8160543 I changed the
>>> behaviour for some calls in c1 generated code to make it comply with
>>> the VM-spec - LinkageError must be thrown before NPE. One detail was
>>> missed in that fix: Some calls, that was static bound and became
>>> optimized virtual calls, now became vanilla virtual calls. The
>>> virtual calls use the unverified entry, but for the static_bound
>>> calls the reciever_klass was not set making the receiver check fail
>>> on every invocation. This scenario is only common with Xcomp when
>>> methods are compiled that have call targets that have not been run
>>> earlier. Also a c1 thing, c2 uses uncommontraps for this case. Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8160543 Webrev:
>>> http://cr.openjdk.java.net/~neliasso/8169766/webrev.01/ Regards, Nils
>>> Eliasson
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR(XS): 8169766: c1 + Xcomp reresolving call target every invocation

Nils Eliasson
Thank you, Vladimir!

// Nils

On 2017-08-11 19:09, Vladimir Kozlov wrote:

> On 8/11/17 7:42 AM, Nils Eliasson wrote:
>> Hi Vladimir,
>>
>>
>> On 2017-08-07 18:42, Vladimir Kozlov wrote:
>>> Good.
>>>
>>> Question. Both, this and 8160543, bugs talk about C1 only. Does the
>>> problem exist with C2 too?
>>
>> The problem only exists with C1. The code is shared with C2, but C2
>> won't be affected because it never tries to emit optimized calls to
>> unloaded methods.
>>
>> (Static bound methods that are loaded have the is_optimized flag set
>> - so they still become optimized calls)
>>
>>> Comments in compiledIC.cpp also talk about C1. Should we guard
>>> 8160543 and this changes in compiledIC.cpp with
>>> if (method_code->is_compiled_by_c1()) ?
>>
>> No, I am removing the c1 specific parts, what's left is used by both.
>>
>> The static_bound argument isn't used anymore and can be removed from
>> the function call:
>>
>> http://cr.openjdk.java.net/~neliasso/8169766/webrev.02/
>
> Good.
>
> Thanks,
> Vladimir
>
>>
>> If I put a diff of the two changes in this function together it
>> becomes more clear why the first one was insufficient.
>>
>>
>> - if (static_bound || is_optimized) {
>> + if (is_optimized) {
>> entry = method_code->verified_entry_point();
>> } else {
>> entry = method_code->entry_point(); }
>> .... - info.set_compiled_entry(entry, (static_bound || is_optimized)
>> ? NULL : receiver_klass, is_optimized);
>> + info.set_compiled_entry(entry, is_optimized ? NULL :
>> receiver_klass, is_optimized);
>>
>>
>> Thanks for having a look,
>> Nils
>>
>>> Thanks, Vladimir On 8/7/17 7:16 AM, Nils Eliasson wrote:
>>>> Hi, Please review this patch. In bug
>>>> https://bugs.openjdk.java.net/browse/JDK-8160543 I changed the
>>>> behaviour for some calls in c1 generated code to make it comply
>>>> with the VM-spec - LinkageError must be thrown before NPE. One
>>>> detail was missed in that fix: Some calls, that was static bound
>>>> and became optimized virtual calls, now became vanilla virtual
>>>> calls. The virtual calls use the unverified entry, but for the
>>>> static_bound calls the reciever_klass was not set making the
>>>> receiver check fail on every invocation. This scenario is only
>>>> common with Xcomp when methods are compiled that have call targets
>>>> that have not been run earlier. Also a c1 thing, c2 uses
>>>> uncommontraps for this case. Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8160543 Webrev:
>>>> http://cr.openjdk.java.net/~neliasso/8169766/webrev.01/ Regards,
>>>> Nils Eliasson

Loading...