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

[Tobias Hartmann]

Hi Christian,

this looks good to me too. Thanks for these detailed explanations.

Best regards,
Tobias

On 29.10.19 08:02, Christian Hagedorn wrote:
> Thank you Vladimir for the review!
> 
> Best regards,
> Christian
> 
> On 28.10.19 16:26, Vladimir Kozlov wrote:
>> 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
>>