[14] RFR(S): 8230019: [REDO] compiler/types/correctness/* tests fail with "assert(recv == __null || recv->is_klass()) failed: wrong type"

[Vladimir Kozlov]

Yes, that is what I suspected. MDO had only one receiver and if we clean counter it will look like it is never called.
clear_row() assumes that at least one receiver is valid.

Hmm, but if the class is unloaded when we construct copy of MDO then this compilation is invalid anyway. Right? You need 
to get new profiling information after unloading to get correct MDO.

If one klass in MDO call site is still valid we can still compile and have uncommon trap for second case. But if no 
valid klass is recorded in MDO call site (all were nulled) we can't compile it based on counter. We should bailout and 
re-collect profiling info or generate uncommon trap instead of call. For later we have to separate cases when the site 
is never called or it was called but classes later were unloaded. hmm, C2 actually should generate uncommon trap if 
class is unloaded. Why classes are unloaded in a first place?

Can you tell why class is unloaded in your test case? It is String!!! Or more accurate why clear_row() was called?

Thanks,
Vladimir

On 10/25/19 10:18 AM, Christian Hagedorn wrote:
> Hi Vladimir
> 
> Thank you for your review!
> 
> On 24.10.19 18:55, Vladimir Kozlov wrote:
>> I would like to see what happens with MDO data with Tobias fix which caused regression. If it is simple. May be from 
>> LogCompilation.
> 
>> The only explanation I can think of if we had only one receiver in MDO which we cleaned up and it confuse C2 to treat 
>> call site as never executed [3] so that it can't use speculative_receiver_type which come from other data
> It was actually my fix. I've tried to come up with a test case where the regression can easily be seen. I found the 
> following example which shows the benefit of not clearing the counter with clear_row():
> 
>      public void test() {
>          String s = "test";
>          for (int i = 0; i < 1_000_000; i++ ) {
>              s.replace("e", "t"); // Just a big enough method that does not get inlined by default
>          }
>      }
> 
> If we use clear_row(), then String::replace is not inlined:
> 
> org.sample.ClearRow::test @ 5 (27 bytes)   made not entrant
>    @ 16   java.lang.String::replace (255 bytes)   too big
> 
> The MDO counter for String::replace is reset to zero. This profile information is passed to ok_to_inline [1] and the 
> zeroed counter is eventually read at [2]. The following frequency check [3] fails and thus InlineTree::should_inline 
> returns false because the method is too big (and wrongly assumed to be never called).
> 
> However, if we just use set_receiver(row, NULL) instead of clear_row(), which does not reset the counter, then 
> call_site_count is big number greater than zero at [2] and the check at [3] succeeds. The maximum inline size is 
> increased to FreqInlineSize (325 by default). Therefore String::replace is inlined:
> 
> org.sample.ClearRow::test @ 5 (27 bytes)   made not entrant
>    @ 16   java.lang.String::replace (255 bytes)   inline (hot)
> 
> 
> I also quickly put it into a local JMH benchmark. It can clearly be seen that not clearing the counter is beneficial:
> 
> set_receiver(row, NULL):
> Benchmark       Mode  Cnt   Score   Error  Units
> ClearRow.test  thrpt    5  42.810 ± 1.572  ops/s
> 
> clear_row():
> Benchmark       Mode  Cnt   Score   Error  Units
> ClearRow.test  thrpt    5  32.658 ± 1.267  ops/s
> 
> 
> There are other cases, for example by using HashMap::put instead, in which call_does_dispatch is true [4]. It then skips 
> directly to your mentioned check at [5]. The check succeeds having a non reset counter and the speculative_receiver_type 
> can be used to inline the method.
> 
> 
> So I guess we've seen large performance regressions due to many such missed inline opportunities due to lost/wrong 
> profiling information.
> 
> Best regards,
> Christian
> 
> 
> [1] http://hg.openjdk.java.net/jdk/jdk/file/18c246ad2ff9/src/hotspot/share/opto/doCall.cpp#l171
> [2] http://hg.openjdk.java.net/jdk/jdk/file/18c246ad2ff9/src/hotspot/share/opto/bytecodeInfo.cpp#l163
> [3] http://hg.openjdk.java.net/jdk/jdk/file/18c246ad2ff9/src/hotspot/share/opto/bytecodeInfo.cpp#l170
> [4] http://hg.openjdk.java.net/jdk/jdk/file/18c246ad2ff9/src/hotspot/share/opto/doCall.cpp#l167
> [5] http://hg.openjdk.java.net/jdk/jdk/file/18c246ad2ff9/src/hotspot/share/opto/doCall.cpp#l205