Append warnings extracted before tail call execution #6849

hubertp · 2023-05-25T14:10:19Z

Pull Request Description

Throwing TailCallException meant that exceptions that were extracted from the expression before the call was made could not be appended. This change catches the TailCallException, adds warnings to it and propagates it further, thus ensuring that we don't loose the information.

Closes #6765.

Important Notes

Removed workarounds introduced in stdlib.

Checklist

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

All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Throwing `TailCallException` meant that exceptions that were extracted from the expression before the call was made could not be appended. This change catches the `TailCallException`, adds warnings to it and propagates it further, thus ensuring that we don't loose the information. Closes #6765.

radeusgd · 2023-05-25T14:21:53Z

...time/src/main/java/org/enso/interpreter/node/callable/dispatch/LoopingCallOptimiserNode.java

+        if (warnings == null) {
+          warnings = e.getWarnings();
+        }


What if warnings != null?

Shouldn't we somehow concatenate the existing warnings with the new ones?

radeusgd

Thanks for removing the outdated workarounds. The tests look all good.

I just have one question as I'm not sure if I correctly understand a piece of logic. Other than that - looks good.

4e6

Every time I look at the engine internals and realize how much bookkeeping is done behind the scenes. I genuinely wonder how it keeps maintaining a decent performance 😄

hubertp · 2023-05-25T14:49:34Z

Every time I look at the engine internals and realize how much bookkeeping is done behind the scenes. I genuinely wonder how it keeps maintaining a decent performance smile

Warnings affect performance. Quite significantly TBH (benchmarks show it).

4e6 · 2023-05-25T14:59:46Z

JaroslavTulach

This is not what I envisioned. I don't understand why TCE should have Warnings field.

I wrote:

How such "loosing of warnings" can happen? v.fold 0 (+) in do_fold_1 strips warnings off and calls fold on "real v" value.
fold uses @Tail_Call and then it returns. The stripped off warnings aren't added.
But how do you want to carry warnings in TailCallException? They are already stripped off when it is created!
The fix is: When dealing with Warnings, catch TailCallException and re-throw with warnings re-attached.

JaroslavTulach · 2023-05-25T16:36:31Z

What are the benchmark results? Is regular TCO slower?

engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java

...time/src/main/java/org/enso/interpreter/node/callable/dispatch/LoopingCallOptimiserNode.java

...ntime/src/main/java/org/enso/interpreter/node/callable/dispatch/SimpleCallOptimiserNode.java

hubertp · 2023-05-26T08:47:17Z

What are the benchmark results?

https://enso-org.github.io/engine-benchmark-results/
The last ones show it - Vectors w/ vs w/o warnings. I remember us talking about it and saying that this will need a bit more work but it is not a priority.

Is regular TCO slower?

No

hubertp · 2023-05-26T08:54:24Z

I wrote:

How such "loosing of warnings" can happen? v.fold 0 (+) in do_fold_1 strips warnings off and calls fold on "real v" value.
fold uses @Tail_Call and then it returns. The stripped off warnings aren't added.
But how do you want to carry warnings in TailCallException? They are already stripped off when it is created!
The fix is: When dealing with Warnings, catch TailCallException and re-throw with warnings re-attached.

Can't re-attach warnings to the result of method call because there is no result to attach to at that point in time. That's why I enhanced TCE to carry that info when the result is available.

Always dispatch warnings in a non-tail position. Can't use `@Cached` because then the underlying function node is not recorded as the child and won't have its ID changed.

hubertp · 2023-05-29T10:08:43Z

@JaroslavTulach Couldn't change to @Cached because of the instrumentation. There is a WarningInstrumentatiopnTest illustrating the problem.

JaroslavTulach

After 5628c71 I see just three changes setting NOT_TAIL:

That seems like the right fix.

…le-6756-6804 * develop: (22 commits) Coalesce graph editor view invalidations (#6786) Append warnings extracted before tail call execution (#6849) Detect and override hooks of the same kind (#6842) Dynamic app resampling and better performance measurements. (#6595) Show spinner when opening/creating a project, take #2 (#6827) Infrastructure for testing inter project imports and exports (#6840) Only initialise visualisation chooser if it is used. (#6758) Allow casting a Mixed column into a concrete type (#6777) Stop graph editing when in full-screen visualization mode (#6844) Handle `show-dashboard` event (#6837) Fix some dashboard issues (#6668) Fix JWT leak (#6815) Fix "set username" screen (#6824) Fallback to opened date when ordering projects (#6814) Various test improvements to increase coverage and speed things up (#6820) do not activate nested dropdowns together (#6830) Clearly select single specialization with enum dispatch pattern (#6819) Prevent incorrect application of list widget on incompatible expressions (#6771) Update GraalVM to 22.3.1 JDK17 (#6750) Import/export syntax error have more specific messages (#6808) ...

Partially revert #6849, which introduced a regression in TCO in the presence of warnings. Rather than modifying the tail call status, `TailCallException` now propagates the extracted warnings and appends them to the final result. Compare to the previous attempt we don't pay the penalty of adding the warnings or even checking for them because it is being dealt in a separate specialization.

Partially revert #6849, which introduced a regression in TCO in the presence of warnings. Rather than modifying the tail call status, `TailCallException` now propagates the extracted warnings and appends them to the final result. Closes #7093 # Important Notes Compared to the previous attempt we don't pay the penalty of adding the warnings or even checking for them because it is being dealt in a separate specialization.

hubertp added 2 commits May 25, 2023 15:56

Remove workarounds

cf7cf2a

hubertp requested review from jdunkerley, radeusgd, GregoryTravis, 4e6, JaroslavTulach and Akirathan as code owners May 25, 2023 14:10

hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label May 25, 2023

radeusgd reviewed May 25, 2023

View reviewed changes

radeusgd approved these changes May 25, 2023

View reviewed changes

4e6 approved these changes May 25, 2023

View reviewed changes

GregoryTravis approved these changes May 25, 2023

View reviewed changes

Akirathan approved these changes May 25, 2023

View reviewed changes

JaroslavTulach requested changes May 25, 2023

View reviewed changes

JaroslavTulach reviewed May 26, 2023

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed May 26, 2023

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/CurryNode.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed May 26, 2023

View reviewed changes

...time/src/main/java/org/enso/interpreter/node/callable/dispatch/LoopingCallOptimiserNode.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed May 26, 2023

View reviewed changes

...ntime/src/main/java/org/enso/interpreter/node/callable/dispatch/SimpleCallOptimiserNode.java Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request May 29, 2023

Could not resolve engine version for Edition(Some(NaN) warnings #6806

Closed

Simplify

2aa35fc

Always dispatch warnings in a non-tail position. Can't use `@Cached` because then the underlying function node is not recorded as the child and won't have its ID changed.

hubertp requested a review from JaroslavTulach May 29, 2023 10:07

nits

5628c71

JaroslavTulach approved these changes May 29, 2023

View reviewed changes

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label May 29, 2023

mergify bot merged commit 6eb4737 into develop May 29, 2023

mergify bot deleted the wip/hubert/6765-warnings-and-tail-calls branch May 29, 2023 12:44

unfurl-links bot mentioned this pull request May 30, 2023

Clearly report type errors on API boundary #6682

Closed

hubertp mentioned this pull request Jun 22, 2023

TCO is not properly applied when function encounters a value with warnings attached #7093

Closed

hubertp mentioned this pull request Jun 23, 2023

Improve TCO in the presence of warnings #7116

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append warnings extracted before tail call execution #6849

Append warnings extracted before tail call execution #6849

hubertp commented May 25, 2023

radeusgd May 25, 2023

radeusgd left a comment

4e6 left a comment

hubertp commented May 25, 2023

4e6 commented May 25, 2023

JaroslavTulach left a comment

JaroslavTulach commented May 25, 2023

hubertp commented May 26, 2023 •

edited

Loading

hubertp commented May 26, 2023

hubertp commented May 29, 2023

JaroslavTulach left a comment

Append warnings extracted before tail call execution #6849

Append warnings extracted before tail call execution #6849

Conversation

hubertp commented May 25, 2023

Pull Request Description

Important Notes

Checklist

radeusgd May 25, 2023

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

4e6 left a comment

Choose a reason for hiding this comment

hubertp commented May 25, 2023

4e6 commented May 25, 2023

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach commented May 25, 2023

hubertp commented May 26, 2023 • edited Loading

hubertp commented May 26, 2023

hubertp commented May 29, 2023

JaroslavTulach left a comment

Choose a reason for hiding this comment

hubertp commented May 26, 2023 •

edited

Loading