RFR: 8275908: Record null_check traps for calls and array_check traps in the interpreter

[Christian Hagedorn]

On Wed, 24 Nov 2021 16:33:35 GMT, Volker Simonis <simonis at openjdk.org> wrote:

> `null_checks` occurring at invoke bytecodes are currently not recorded by the profiler. This leads to unnecessary uncommon traps, deoptimizations and recompilations for exceptions which already occurred before the compilation (i.e. are "hot"). This change fixes the problem in the interpreter.
> 
> `array_checks` are currently recorded as `class_checks` in the interpreter and therefore not recognized by the compiler. This again leads to uncommon traps, deoptimizations and recompilations. This change unifies the handling of `array_checks` in the interpreter and compiler and prevents unnecessary recompilation.
> 
> The test is a stripped down version of a test which was developed for [JDK-8273563: Improve performance of implicit exceptions with -XX:-OmitStackTraceInFastThrow](https://bugs.openjdk.java.net/browse/JDK-8273563) (still [under review](https://github.com/openjdk/jdk/pull/5488)). It introduces an extension to the Whitebox API to expose the decompile, deopt and trap counters which is also required for testing [JDK-8273563](https://bugs.openjdk.java.net/browse/JDK-8273563). I think (and hope) it will also be helpful for others in the future.

Otherwise, it looks good to me! But would be good to get a second review for it.

Nice test!

src/hotspot/share/interpreter/interpreterRuntime.cpp line 834:

> 832:                                  THREAD);
> 833: 
> 834:     if(HAS_PENDING_EXCEPTION) {

Missing space

src/hotspot/share/opto/parseHelper.cpp line 301:

> 299: 
> 300: #endif
> 301: 

This line was probably deleted by mistake?

src/hotspot/share/runtime/deoptimization.hpp line 436:

> 434: 
> 435:   static jint total_deoptimization_count();
> 436:   static jint deoptimization_count(const char *reason_str, const char *action_str);

Nit: Asterisk should be at the type: `const char* reason_str`

test/hotspot/jtreg/compiler/exceptions/OptimizeImplicitExceptions.java line 141:

> 139:     private static void printCounters(TestMode testMode, ImplicitException impExcp, Method throwImplicitException_m, int invocations) {
> 140:         System.out.println("testMode=" + testMode + " exception=" + impExcp + " invocations=" + invocations + "\n" +
> 141:                            "decompilecount=" + WB.getMethodDecompileCount(throwImplicitException_m) + " " +

`getMethodDecompileCount()` seems only to be used to print the counters here but is not verified otherwise. If it is is not too complicated, could a specific test for it be added as well?

test/hotspot/jtreg/compiler/exceptions/OptimizeImplicitExceptions.java line 166:

> 164:     // Checks after the JIT-compiled test method has been invoked 'PerBytecodeTrapLimit' times.
> 165:     private static void checkTwo(TestMode testMode, ImplicitException impExcp, Exception ex, Method throwImplicitException_m, int invocations) {
> 166: 

If I see that correctly, `checkTwo`, `checkThree` and `checkFour` only differ in whether using `PerBytecodeTrapLimit` or `Tier0InvokeNotifyFreq` and could be merged together (if the omitted assertions in `checkThree` and `checkFour` for the exception message compared to `checkTwo` are valid to be added again).

test/hotspot/jtreg/compiler/exceptions/OptimizeImplicitExceptions.java line 287:

> 285:                 checkTwo(testMode, impExcp, lastException, throwImplicitException_m, invocations);
> 286: 
> 287:                 // Invoke compiled (or interpreted if JDK-8275908 isn't fixed) code 'Tier0InvokeNotifyFreq' times.

As this is the fix for JDK-8275908, you can remove the comment about it :-) It's probably a leftover from JDK-
8273563.

test/lib/sun/hotspot/WhiteBox.java line 321:

> 319:     return getMethodCompilationLevel0(method, isOsr);
> 320:   }
> 321:   public         int     getMethodDecompileCount(Executable method) {

As this class is marked as `@Deprecated`, do we need to add the methods here as well?

-------------

PR: https://git.openjdk.java.net/jdk/pull/6541