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

[Christian Hagedorn]

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