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

Tue Sep 14 07:11:37 UTC 2021

(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
(see by example [1] on stackoverflow).

This is not a new issue, this bug pop time to time since OmitStackTraceInFastThrow was introduced (in 1.4.x, i believe).

Thanks to taking the time to fix that.

regards,
Rémi

[1] https://stackoverflow.com/questions/2411487/nullpointerexception-in-java-with-no-stacktrace

----- Original Message -----
> From: "Volker Simonis" <volker.simonis at gmail.com>
> To: "Volker Simonis" <simonis at openjdk.java.net>
> Cc: "hotspot-dev" <hotspot-dev at openjdk.java.net>, "hotspot compiler" <hotspot-compiler-dev at openjdk.java.net>
> Sent: Lundi 13 Septembre 2021 10:08:40
> Subject: Re: RFR: 8273392: Improve usability of stack-less exceptions due to -XX:+OmitStackTraceInFastThrow

> Hi,
> 
> may I kindly ask somebody to please take a look at this PR?
> 
> Thank you and best regards,
> Volker
> 
> On Tue, Sep 7, 2021 at 5:42 PM Volker Simonis <simonis at openjdk.java.net> 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.
>>
>> -------------
>>
>> Commit messages:
>>  - 8273392: Improve usability of stack-less exceptions due to
>>  -XX:+OmitStackTraceInFastThrow
>>
>> Changes: https://git.openjdk.java.net/jdk/pull/5392/files
>>  Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5392&range=00
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8273392
>>   Stats: 538 lines in 12 files changed: 417 ins; 6 del; 115 mod
>>   Patch: https://git.openjdk.java.net/jdk/pull/5392.diff
>>   Fetch: git fetch https://git.openjdk.java.net/jdk pull/5392/head:pull/5392
>>
> > PR: https://git.openjdk.java.net/jdk/pull/5392