Eliminate handler frames from `Caught_Panic.stack_trace` #12024

JaroslavTulach · 2025-01-09T06:54:57Z

Pull Request Description

Fixes #8153 by

creating a test that demonstrates the problem - c33d647
creating a simple fix - 0393b22
measuring impact of a simple fix on performance - done
refining the fix to avoid negative performance impact - c188d97

Important Notes

The expected refinement of the fix is going to:

record CatchPanicNode in PanicException when it is caught
add @ExportMessage Object getExceptionStackTrace() to control the stack trace of PanicException
when computing and recording getExceptionStackTrace of the PanicException skip frames between...
... the node that is computing the stack and the recorded CatchPanicNode anchor

This will fix the most common scenario when a caught_exception.stack_trace is requested in the exception handler.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

All code follows the
Scala,
Java,
Unit tests have been written where possible.

JaroslavTulach · 2025-01-09T06:55:54Z

creating a test that demonstrates the problem

Without any fix the test introduced by c33d647 fails on the CI:

Frames from exception handling should not be in the stacktrace:
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines<arg-0> (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:45:17-42)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.f5 (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:37:21-21)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.f4 (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:38:21-27)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.f3<arg-1> (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:41:21-27)
at Caught_Panic.stack_trace (built-distribution\enso-engine-2025.1.1-dev-windows-amd64\enso-2025.1.1-dev\lib\Standard\Base\2025.1.1-dev\src\Panic.enso:284:9-43)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.do_the_handling (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:30:22-45)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.my_exception_handler (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:35:17-44)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.f3 (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:40-41)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.f2 (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:42:21-27)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines.f1 (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:43:21-27)
at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines (test\Base_Tests\src\Runtime\Stack_Traces_Spec.enso:45:13-43)

because it contains forbidden frames do_the_handling and my_exception_handler. After 0393b22 the test should be fixed, but we have to verify the benchmarks:

Results:

some Panic benchmarks were created as part of Compare performance of Panic / Java Exception / Dataflow error #8130
one of them really regressed after 0393b22

Good! Time to speed it up.

test/Base_Tests/src/Runtime/Stack_Traces_Spec.enso

JaroslavTulach · 2025-01-09T16:48:36Z

Local benchmarking indicates everything is OK, but running final verification on the CI as well.

sbt:enso> std-benchmarks/benchOnly Panics_And_Errors
[info] Benchmark                               Mode  Cnt  Score   Error  Units
[info] Panics_And_Errors_10000.Dataflow_Error  avgt    2  0.052          ms/op
[info] Panics_And_Errors_10000.Panic           avgt    2  0.449          ms/op

Just like it always was.

radeusgd · 2025-01-09T17:44:59Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/PanicException.java

+  /**
+   * Whoever catches a {@code PanicException} should associate its location with it, so proper stack
+   * traces can be computed later. Because the stacktrace information is relative to the caught
+   * location
+   *
+   * @param node the node who caught the panic
+   */
+  public final void assignCaughtLocation(CatchPanicNode node) {
+    this.caughtBy = node;
+  }


What happens if the panic is rethrown and caught again elsewhere? Should this keep the first catch location or be reassigned?

That's a very good question. It points out that this all is just a heuristic with the goal (as stated in the description):

... fix the most common scenario when a caught_exception.stack_trace is requested in the exception handler

I will add a new

test with re-throw.

done here

Should this ... be reassigned?

Both CatchPanicNode and Node queryNode have to be on the stack for any transformation to happen. Thus yes, it has to be reassigned.

I will also add a test that

catches the panic

queries the stack_trace in the handler

stores the panic into a Vector

and then queries later

both stacks have to be identical

Done in 9a056e3

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/PanicException.java

test/Base_Tests/src/Runtime/Stack_Traces_Spec.enso

Co-authored-by: Radosław Waśko <[email protected]>

…race8153

JaroslavTulach · 2025-01-10T07:16:07Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/CatchPanicNode.java

+      } else {
+        // materializes stack trace of non-Enso
+        // exceptions in case it is needed by the handler
+        TruffleStackTrace.fillIn(originalException);


This path is now tested by a5d001b that throws JavaScript exception and checks its stacktrace.

JaroslavTulach · 2025-01-10T08:32:49Z

test/Base_Tests/src/Runtime/Stack_Traces_Spec.enso

+        out . should_contain "f4"
+        out . should_contain "f5"
+
+        top_lines.take (..While l-> l.starts_with "2nd:") . length . should_equal 11


The stack trace in this case is:

2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines<arg-1> (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:72:38-63) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.f5 (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:44:17-17) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.f4 (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:45:17-23) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.f3<arg-1> (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:48:17-23) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.rethrow_handler (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:39:13-36) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.f3 (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:47-48) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.f2 (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:49:17-23) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check.f1 (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:50:17-23) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.stack_trace_check (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:52:9-26) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines<arg-1> (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:72:17-77) 2nd: at Stack_Traces_Spec.add_specs.Stack_Traces_Spec.add_specs.lines (Base_Tests/src/Runtime/Stack_Traces_Spec.enso:71-72)

which I believe is exactly what we want.

Looks good I think.

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/PanicException.java

Co-authored-by: Radosław Waśko <[email protected]>

…ment

Test to verify frames of caught Panic

c33d647

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 9, 2025

JaroslavTulach requested review from hubertp and Akirathan January 9, 2025 06:54

JaroslavTulach self-assigned this Jan 9, 2025

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley and marthasharkey as code owners January 9, 2025 06:54

JaroslavTulach marked this pull request as draft January 9, 2025 06:55

Filling in stack trace when PanicException is caught fixes the test

0393b22

JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 9, 2025

radeusgd reviewed Jan 9, 2025

View reviewed changes

test/Base_Tests/src/Runtime/Stack_Traces_Spec.enso Outdated Show resolved Hide resolved

JaroslavTulach added 2 commits January 9, 2025 16:47

Shortening with should_not_contain

a0f3854

assignCaughtLocation to each caught PanicException

c188d97

JaroslavTulach marked this pull request as ready for review January 9, 2025 16:46

JaroslavTulach requested a review from 4e6 as a code owner January 9, 2025 16:46

JaroslavTulach requested a review from radeusgd January 9, 2025 16:48

radeusgd reviewed Jan 9, 2025

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/PanicException.java Outdated Show resolved Hide resolved

radeusgd reviewed Jan 9, 2025

View reviewed changes

test/Base_Tests/src/Runtime/Stack_Traces_Spec.enso Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request Jan 10, 2025

caught_panic.stack_trace includes stack frames from the handler itself #8153

Closed

JaroslavTulach and others added 4 commits January 10, 2025 06:23

More stack traces checks

620a27f

Co-authored-by: Radosław Waśko <[email protected]>

Merge remote-tracking branch 'origin/develop' into wip/jtulach/StackT…

f43e8d0

…race8153

Avoid Text.create(null) when there is no path for a source

3a0a34a

Verify behavior of a JavaScript stack trace

a5d001b

JaroslavTulach commented Jan 10, 2025

View reviewed changes

Verify behavior when rethrown

64e4af5

JaroslavTulach force-pushed the wip/jtulach/StackTrace8153 branch from 11f3266 to 64e4af5 Compare January 10, 2025 08:34

Remember the stacktrace once its computed

9a056e3

JaroslavTulach requested a review from radeusgd January 10, 2025 08:51

radeusgd reviewed Jan 10, 2025

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/PanicException.java Show resolved Hide resolved

radeusgd approved these changes Jan 10, 2025

View reviewed changes

Akirathan approved these changes Jan 10, 2025

View reviewed changes

JaroslavTulach and others added 3 commits January 10, 2025 13:46

Link by Radek

bf0a1b3

Co-authored-by: Radosław Waśko <[email protected]>

Avoid -1 and use toArray

f33ea8e

Using artificial exception to estimate the depth of a stack at the mo…

5a8f973

…ment

JaroslavTulach force-pushed the wip/jtulach/StackTrace8153 branch from 67667bc to 5a8f973 Compare January 10, 2025 13:58

JaroslavTulach merged commit 7d4110d into develop Jan 10, 2025
47 checks passed

JaroslavTulach deleted the wip/jtulach/StackTrace8153 branch January 10, 2025 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate handler frames from `Caught_Panic.stack_trace` #12024

Eliminate handler frames from `Caught_Panic.stack_trace` #12024

JaroslavTulach commented Jan 9, 2025 •

edited

Loading

JaroslavTulach commented Jan 9, 2025 •

edited

Loading

JaroslavTulach commented Jan 9, 2025

radeusgd Jan 9, 2025

JaroslavTulach Jan 10, 2025 •

edited

Loading

JaroslavTulach Jan 10, 2025 •

edited

Loading

JaroslavTulach Jan 10, 2025

radeusgd Jan 10, 2025

Eliminate handler frames from Caught_Panic.stack_trace #12024

Eliminate handler frames from Caught_Panic.stack_trace #12024

Conversation

JaroslavTulach commented Jan 9, 2025 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach commented Jan 9, 2025 • edited Loading

JaroslavTulach commented Jan 9, 2025

radeusgd Jan 9, 2025

Choose a reason for hiding this comment

JaroslavTulach Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jan 10, 2025

Choose a reason for hiding this comment

radeusgd Jan 10, 2025

Choose a reason for hiding this comment

Eliminate handler frames from `Caught_Panic.stack_trace` #12024

Eliminate handler frames from `Caught_Panic.stack_trace` #12024

JaroslavTulach commented Jan 9, 2025 •

edited

Loading

JaroslavTulach commented Jan 9, 2025 •

edited

Loading

JaroslavTulach Jan 10, 2025 •

edited

Loading

JaroslavTulach Jan 10, 2025 •

edited

Loading