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

[Christian Hagedorn]

On Thu, 25 Nov 2021 10:52:02 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> src/hotspot/share/opto/parseHelper.cpp line 301:
>> 
>>> 299: 
>>> 300: #endif
>>> 301: 
>> 
>> This line was probably deleted by mistake?
>
> The line was actually deleted by my editor. I first wondered myself, but the file had an extra empty line et the end which I think we discourage. So at the end I think my editor was right :)
> But as there remained no other changes in that file except the deleted empty line I agree that it looks strange now and I'll restore the file to its initial state.

I agree that we should then remove this line to follow the convention but as you've said, it might not be justified to fix these things in otherwise untouched files. Maybe a thing to fix for the next one who edits this file ;)

>> 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?
>
> Hm, it was used in JDK-8275908 before this fix. Now, with this fix we don't get any recompiles any more :)
> 
> The tests is already quite big so I've added a separate test `test/hotspot/jtreg/compiler/uncommontrap/Decompile.java` to verify the new WhiteBox methods introduced by  this change.

Thanks for the effort to add an extra extensive test for it, it looks good!

>> 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).
>
> That's a good point, now that the tests have got a little simpler :)
> 
> I'll have to add more cases for JDK-8273563 but I think it's just fair to leave it for that change.

That sounds good.

>> 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?
>
> Unfortunately yes :(
> I first didn't but there are still tests using it and they'll get warning otherwise. To make matters worse, some of them parse the output, so I gave up and added the methods to `sun/hotspot/WhiteBox.java` as well.

Oh I see, that's indeed unfortunate. I wasn't aware of that. Then better leave them in until the entire class gets removed at some point. Thanks for the explanation!

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

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