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

Tue Oct 29 13:18:03 UTC 2019

Thank you for the review Tobias!

Best regards,
Christian

On 29.10.19 12:16, Tobias Hartmann wrote:
> 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
>>>