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

[Vladimir Kozlov]

Okay, now I got it what was wrong with previous fix. Thank you for explaining.
The current fix is good.

Thanks
Vladimir

> On Oct 28, 2019, at 3:38 AM, Christian Hagedorn <christian.hagedorn at oracle.com> wrote:
> 
> Hi Vladimir
> 
> I don't think that the String class is unloaded. In the previous fix [6] I called clear_row() if either the klass 'k' was NULL or it was unloaded. This means that in my test case the MDO contains the String klass at row index 0 but the index 1 is empty, i.e. the klass is NULL. As a result, I cleared that counter with clear_row() by mistake when processing the row index 1 (since 'k' is NULL). My new fix will only set the klass to NULL with set_receiver(row, NULL) and will not clear the counter anymore. In my evaluation below I compared the previous fix [6] with my new fix.
> 
> Best regards,
> Christian
> 
> [6] http://cr.openjdk.java.net/~thartmann/8225670/webrev.00/
> 
>> On 26.10.19 00:12, Vladimir Kozlov wrote:
>> 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