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

[Christian Hagedorn]

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 
>>