RFR(XS) 8191688: Assert failed in > 200 tests: failed dependencies, but counter didn't change

[dean.long at oracle.com]

On 11/22/17 4:13 AM, Vladimir Ivanov wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8191688
>> http://cr.openjdk.java.net/~dlong/8191688/webrev/
>
> The fix looks good.
>

Thanks Vladimir.  I already pushed it, so I wasn't able to add you as a 
reviewer.

> An observation not related to the bug:
>
> +  if (env->jvmti_can_hotswap_or_post_breakpoint()) {
>      // 6328518 check hotswap conditions under the right lock.
>      MutexLocker locker(Compile_lock);
>      if (Dependencies::check_evol_method(h_m()) != NULL) {
>        _is_c1_compilable = false;
>        _is_c2_compilable = false;
> +      _can_be_parsed = false;
>      }
>    } else {
>
> Klass* Dependencies::check_evol_method(Method* m) {
> ...
>   if (m->is_old()
>       || m->number_of_breakpoints() > 0) {
>     return m->method_holder();
>
> It seems once there's a breakpoint set in a method, it will never be 
> compiled again. (I looked in the code couldn't find where 
> _is_c1_compilable/_is_c2_compilable are reset once all breakpoints are 
> cleared). If that's the case, it has severe performance consequences, 
> so better to fix it.
>

The ciMethod only lasts for the lifetime of the compiler task, right?

> Also, some nitpicking follows:
>
> +  bool _can_be_parsed;
>
> +  bool can_be_parsed() const { return _can_be_parsed; }
>
> The name confuses me. I'd presume it signals there are some problems 
> with parsing the method, so don't even try. It seems the choice was 
> affected by InlineTree::check_can_parse(), but the latter looks 
> justified:
>
> const char* InlineTree::check_can_parse(ciMethod* callee) {
>   // Certain methods cannot be parsed at all:
>   if ( callee->is_native())                     return "native method";
>   if ( callee->is_abstract())                   return "abstract method";
>   if (!callee->has_balanced_monitors())         return "not compilable 
> (unbalanced monitors)";
>   if ( callee->get_flow_analysis()->failing())  return "not compilable 
> (flow analysis failed)";
>
> Why doesn't it stress it's all about inlining? E.g., 
> ciMethod::can_be_inlined looks more descriptive in that context.
>

The way things are now, "can be parsed" seems to mean "can be compiled 
or inlined".  If there was a way to say a method can be compiled but not 
inlined, then I agree can_be_inlined would make sense.

dl

> Best regards,
> Vladimir Ivanov