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

[Richard Reingruber]

On Tue, 7 Sep 2021 15:25:46 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.

Hi Volker, Rémis,

I haven't yet looked into the details of the change but @TheRealMDoerr kindly
explained it to me. As I understood, you are using global JNI references to hold
the preallocated exceptions with partial backtrace. Backtraces seem to hold
references to the java mirrors of the holders of the methods in the backtrace
[1]. This will keep their classloaders alive and prevent classunloading. Also
the owning nmethod cannot be unloaded for the same reason. It shouldn't be to
difficult to write a test that leaks classes because of this.

> 
> _Mailing list message from [Remi Forax](mailto:forax at univ-mlv.fr) on [hotspot-dev](mailto:hotspot-dev at mail.openjdk.java.net):_
> 
> (not a reviewer so this message will not be really helpful ...)
> 
> Hi Volker,
> for me it's not an enhancement, but a bug fix, in production an exception with no stacktrace is useless and result in hours lost trying to figure out the issue


I'd agree. Even in development exceptions should have a stacktrace and
therefore, IMHO, OmitStackTraceInFastThrow should be off by default.


In my eyes exceptions are means to handle unforseen application states in an
best effort approach. Often they will be caused by bugs and the attached
stacktrace is valuable information to find them. Your enhancement limits the
stacktrace to potentially just the top frame which in many cases will not be
enough and also confusing to developers. Also I don't think that we should
optimize applications that have run into a bug.

In the related [JDK-8273563](https://bugs.openjdk.java.net/browse/JDK-8273563)
you gave the example of [Tomcat's `HttpParser::isAlpha()`
method](https://github.com/apache/tomcat/blob/26ba86cdbd40ca718e43b82e62b3eb49d004c3d6/java/org/apache/tomcat/util/http/parser/HttpParser.java#L266-L274)
method:


    public static boolean isAlpha(int c) {
        try {
            return IS_ALPHA[c];
        } catch (ArrayIndexOutOfBoundsException ex) {
            return false;
        }
    }


There the backtrace is completely useless and can be omitted but IMHO
(stated above) this is a misuse of exceptions in the first place and should be
fixed.

Such idioms may occur in libraries that are out of maintenance (which should not
be used for security reasons) or the maintainer is not willing to accept the
fix. Therefore we could limit OmitStackTraceInFastThrow to these idioms only
where the thrown exception is caught in a local or inlined handler that ignores
it.

Maybe this is not even too difficult. If C2 compiles `isAlpha` with
OmitStackTraceInFastThrow enabled then practically everything related to
exception handling gets eliminated. It might be possible to recognize that the
IR-Node representing the preallocated exception has no uses and only if it
actually does have uses it could be replaced with an uncommon trap (which is
likely the harder part).

Cheers, Richard.

[1] Exception backtrace references java mirrors:
    https://github.com/openjdk/jdk/blob/7c9868c0b3c9bd3d305e71f91596190813cdccce/src/hotspot/share/classfile/javaClasses.cpp#L2178-L2182

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

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