RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

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

RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

Aleksey Shipilev-4
Bug:
  https://bugs.openjdk.java.net/browse/JDK-8206931

It seems that JDK-8191052 made a misleading change:
  http://hg.openjdk.java.net/jdk/hs/rev/e8f5fc8f5f67

We used to print "invalid non-klass dependency" when we have detected the dependency that failed
is_klass_type() check. New code has Dependencies::validate_dependencies to reply with klass
dependency when SystemDictionary was changed and non-klass dependency when such dependency was
detected. But caller in ciEnv only prints "invalid non-klass dependency" when dependency _is_ the
klass dependency. jvmciEnv uses it inappropriately too, even though without observable effects.

Fix:
  http://cr.openjdk.java.net/~shade/8206931/webrev.01/

Testing: tier1_compiler, eyeballing compiler logs

Thanks,
-Aleksey


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

Re: RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

Vladimir Ivanov
src/hotspot/share/ci/ciEnv.cpp:

IMO the code is more readable when the check is left as is, but the
messages are swapped instead:

      } else if (Dependencies::is_klass_type(result)) {
        record_failure("concurrent class loading");
      } else {
        record_failure("invalid non-klass dependency");
      }

Otherwise, looks good!

Best regards,
Vladimir Ivanov

On 10/07/2018 10:09, Aleksey Shipilev wrote:

> Bug:
>    https://bugs.openjdk.java.net/browse/JDK-8206931
>
> It seems that JDK-8191052 made a misleading change:
>    http://hg.openjdk.java.net/jdk/hs/rev/e8f5fc8f5f67
>
> We used to print "invalid non-klass dependency" when we have detected the dependency that failed
> is_klass_type() check. New code has Dependencies::validate_dependencies to reply with klass
> dependency when SystemDictionary was changed and non-klass dependency when such dependency was
> detected. But caller in ciEnv only prints "invalid non-klass dependency" when dependency _is_ the
> klass dependency. jvmciEnv uses it inappropriately too, even though without observable effects.
>
> Fix:
>    http://cr.openjdk.java.net/~shade/8206931/webrev.01/
>
> Testing: tier1_compiler, eyeballing compiler logs
>
> Thanks,
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

Aleksey Shipilev-4
On 07/10/2018 01:19 PM, Vladimir Ivanov wrote:
> src/hotspot/share/ci/ciEnv.cpp:
>
> IMO the code is more readable when the check is left as is, but the messages are swapped instead:
>
>      } else if (Dependencies::is_klass_type(result)) {
>        record_failure("concurrent class loading");
>      } else {
>        record_failure("invalid non-klass dependency");
>      }

That makes sense:
 http://cr.openjdk.java.net/~shade/8206931/webrev.02/

-Aleksey


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

Re: RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

Tom Rodriguez-2


Aleksey Shipilev wrote on 7/10/18 5:04 AM:

> On 07/10/2018 01:19 PM, Vladimir Ivanov wrote:
>> src/hotspot/share/ci/ciEnv.cpp:
>>
>> IMO the code is more readable when the check is left as is, but the messages are swapped instead:
>>
>>       } else if (Dependencies::is_klass_type(result)) {
>>         record_failure("concurrent class loading");
>>       } else {
>>         record_failure("invalid non-klass dependency");
>>       }
>
> That makes sense:
>   http://cr.openjdk.java.net/~shade/8206931/webrev.02/

Swapping the messages looks better.

You shouldn't change the logic in jvmciEnv.cpp.  It's actually correct.
!is_klass_type(result) basically means result == call_site_target_value
and since that dependence doesn't trigger a counter change we just want
to report a failure instead of reporting it as invalid.

tom

>
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

Aleksey Shipilev-4
On 07/10/2018 05:41 PM, Tom Rodriguez wrote:

>
>
> Aleksey Shipilev wrote on 7/10/18 5:04 AM:
>> On 07/10/2018 01:19 PM, Vladimir Ivanov wrote:
>>> src/hotspot/share/ci/ciEnv.cpp:
>>>
>>> IMO the code is more readable when the check is left as is, but the messages are swapped instead:
>>>
>>>       } else if (Dependencies::is_klass_type(result)) {
>>>         record_failure("concurrent class loading");
>>>       } else {
>>>         record_failure("invalid non-klass dependency");
>>>       }
>>
>> That makes sense:
>>   http://cr.openjdk.java.net/~shade/8206931/webrev.02/
>
> Swapping the messages looks better.
Thanks!

> You shouldn't change the logic in jvmciEnv.cpp.  It's actually correct. !is_klass_type(result)
> basically means result == call_site_target_value and since that dependence doesn't trigger a counter
> change we just want to report a failure instead of reporting it as invalid.

OK, I reverted jvmciEnv.cpp change, here:
  http://cr.openjdk.java.net/~shade/8206931/webrev.03/

-Aleksey


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

Re: RFR (XS) 8206931: Misleading "COMPILE SKIPPED: invalid non-klass dependency" compile log

Vladimir Kozlov
Looks go to me. I will run our regular testing on it and let you know results.

Thanks,
Vladimir

On 7/10/18 8:49 AM, Aleksey Shipilev wrote:

> On 07/10/2018 05:41 PM, Tom Rodriguez wrote:
>>
>>
>> Aleksey Shipilev wrote on 7/10/18 5:04 AM:
>>> On 07/10/2018 01:19 PM, Vladimir Ivanov wrote:
>>>> src/hotspot/share/ci/ciEnv.cpp:
>>>>
>>>> IMO the code is more readable when the check is left as is, but the messages are swapped instead:
>>>>
>>>>        } else if (Dependencies::is_klass_type(result)) {
>>>>          record_failure("concurrent class loading");
>>>>        } else {
>>>>          record_failure("invalid non-klass dependency");
>>>>        }
>>>
>>> That makes sense:
>>>    http://cr.openjdk.java.net/~shade/8206931/webrev.02/
>>
>> Swapping the messages looks better.
>
> Thanks!
>
>> You shouldn't change the logic in jvmciEnv.cpp.  It's actually correct. !is_klass_type(result)
>> basically means result == call_site_target_value and since that dependence doesn't trigger a counter
>> change we just want to report a failure instead of reporting it as invalid.
>
> OK, I reverted jvmciEnv.cpp change, here:
>    http://cr.openjdk.java.net/~shade/8206931/webrev.03/
>
> -Aleksey
>