RFR: 8273392: Improve usability of stack-less exceptions due to -XX:+OmitStackTraceInFastThrow [v3]

[Volker Simonis]

On Tue, 21 Sep 2021 10:09:11 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> If running with `-XX:+OmitStackTraceInFastThrow` (which is the default) C2 will optimize certain "hot" implicit exceptions (i.e. AIOOBE, NullPointerExceptions,..) and replace them by a static, pre-allocated exception without any stacktrace.
>> 
>> However, we can actually do better. Instead of using a single, pre-allocated exception object for all methods we can let the compiler allocate specific exceptions for each compilation unit (i.e. nmethod) and fill them with at least one stack frame with the method /line-number information of the currently compiled method. If the method in question is being inlined (which often happens), we can add stackframes for all callers up to the inlining depth of the method in question.
>> 
>> For the attached JTreg test, we get the following exception in interpreter mode:
>> 
>> java.lang.NullPointerException: Cannot read the array length because "<parameter2>" is null
>>         at compiler.exceptions.StackFrameInFastThrow.throwImplicitException(StackFrameInFastThrow.java:76)
>>         at compiler.exceptions.StackFrameInFastThrow.level2(StackFrameInFastThrow.java:95)
>>         at compiler.exceptions.StackFrameInFastThrow.level1(StackFrameInFastThrow.java:99)
>>         at compiler.exceptions.StackFrameInFastThrow.main(StackFrameInFastThrow.java:233)
>> 
>> Once the method gets compiled with `-XX:+OmitStackTraceInFastThrow` the same exception will look as follows:
>> 
>> java.lang.NullPointerException
>> 
>> After this change, if `StackFrameInFastThrow.throwImplicitException()` will be compiled stand alone, we will get:
>> 
>> java.lang.NullPointerException
>>         at compiler.exceptions.StackFrameInFastThrow.throwImplicitException(StackFrameInFastThrow.java:76)
>> 
>> and if `StackFrameInFastThrow.throwImplicitException()` will be inlined into `level2()` and `level2()` into `level1()` we will get the following exception (altough we're still running with `-XX:+OmitStackTraceInFastThrow`):
>> 
>> java.lang.NullPointerException
>>         at compiler.exceptions.StackFrameInFastThrow.throwImplicitException(StackFrameInFastThrow.java:76)
>>         at compiler.exceptions.StackFrameInFastThrow.level2(StackFrameInFastThrow.java:95)
>>         at compiler.exceptions.StackFrameInFastThrow.level1(StackFrameInFastThrow.java:99)
>> 
>> The new functionality is guarded by `-XX:+/-StackFrameInFastThrow`, but switched on by default (I'll create a CSR for the new option once reviewers are comfortable with the change). Notice that the optimization comes at no run-time costs because all the extra work will be done at compile time.
>> 
>> ## Implementation details
>> 
>> - Already the current implementation of `-XX:+OmitStackTraceInFastThrow` potentially lazy-allocates the empty singleton exceptions like AIOOBE in `ciEnv::ArrayStoreException_instance()`. With this change, if running with `-XX:+StackFrameInFastThrow` we will always allocate new exception objects and populate them with the stack frames which are statically available at compile time (see `java_lang_Throwable::fill_in_stack_trace_of_implicit_exception()`).
>> - Because nmethods don't act as strong GC roots, we have to create a global JNI handle for every newly generated exception to prevent GC from collecting them.
>> - In order to avoid a memory leak we have to release these global JNI handles once a nmethod gets unloaded. In order to achieve this, I've added a new section "implicit exceptions" to the nmethod which holds these JNI handles.
>> - While adding the new  "implicit exceptions" section to the corresponding stats (`print_nmethod_stats()` and printing routines (`nmethod::print()`) I realized that a previous change ([JDK-8254231: Implementation of Foreign Linker API (Incubator)](https://bugs.openjdk.java.net/browse/JDK-8254231)) had already introduced a new nmethod section ("native invokers") but missed to add it to the corresponding stats and printing routines so I've added that section as well.
>> - The `#ifdef COMPILER2` guards are only required to not break the `zero`/`minimal` builds.
>> - The JTreg test is using `-XX:PerMethodTrapLimit=0` to handle all implicit exceptions as "hot". This makes the test simpler and at the same time provokes the allocation of more implicit exceptions.
>> - Manually verified that the created Exception objects are freed by GC once the corresponding nmethods have been flushed.
>> - Manual "stress" test with a very small heap and continuous recompilation of methods with explicit exceptions to provoke GCs during compilation didn't reveal any issues.
>
> Volker Simonis has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Create implcit exceptions with an array of StackTraceElements right away instead of creating a backtrace. This prevents that implicit exceptions will keep classes alive due to Java mirrors in the backtrace.

Hi Jon,

thanks for looking at this PR. Let me reply to your comments inline:

> To me this looks like a very clever mess. The mess comes from the trickiness (it's a tricky problem!) and even more from forcing various parts of the system that are usually isolated to come into contact. Adding reasons to GC during a JIT task is a smell. Adding objects which are pieced together at compile time is a smell. (And you can't run Java code from the JIT; it's an architectural limitation.)

I agree. But we already have all this mess today with the current implementation of `-XX:+OmitStackTraceInFastThrow` which already lazily creates empty exceptions and introduces all the problems you describe (see `ciEnv::get_or_create_exception()`). It's to a much lesser extent compared to this change, but fundamentally it is not different.

> Having JavaClasses talk directly to C2 GraphKit (without even a CI class between) is a smell. Adding a new section to nmethods just to make a poorly-understood life cycle for an odd (non-Java-created) group of exceptions is a smell.
> 
> If we need a new section on nmethods it should be something more like "Java structures the JIT has made", with clearly separated concerns from the rest of the system, rather than "my very special section for an intrusive RFE".
> 

That's a good proposal and I'm happy to work on such a solution. What do you mean exactly by ".._clearly separated concerns from the rest of the system_.."? 

> This is not even close to being ready to integrate.

As I said, I'm happy to invest more work and improve this PR based on your suggestion if there's a chance for this feature to be accepted (even if only in a heavily revised form).

But in general I think the **biggest mess** is really that users still get empty exceptions without any information at all and I think it is time to fix that. Unfortunately I can't see into the history of this code before jdk 6, but from [JDK-4292742: NullPointerException with no stack trace](https://bugs.openjdk.java.net/browse/JDK-4292742) it looks like you already worked on this issue almost 20 years ago :)

So what about removing JavaClasses' dependency on GraphKit and making the new nmethod section more generally usable as you suggested? Are there any other pain points before reconsidering this PR? Any other suggestions you like me to integrate?

Thank you and best regards,
Volker

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

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