Improve TCO in the presence of warnings #7116
Conversation
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.
hubertp
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
4e6,
JaroslavTulach and
Akirathan
as code owners
...time/src/main/java/org/enso/interpreter/node/callable/dispatch/LoopingCallOptimiserNode.java
Outdated
Show resolved
Hide resolved
hubertp
added
the
CI: No changelog needed
| vec = Vector.new 10000 (i-> i+1) | ||
| elem1 = Warning.attach "WARNING1" 998 | ||
| vec.contains 998 . should_equal True | ||
| res1 = vec.contains elem1 |
There was a problem hiding this comment.
Is this the line that was causing stack overflow?
There was a problem hiding this comment.
Yes, this is the example from the ticket.
| @@ -277,7 +287,7 @@ public Object invokeWarnings( | |||
| invokeFunctionNode.getSchema(), | |||
| invokeFunctionNode.getDefaultsExecutionMode(), | |||
| invokeFunctionNode.getArgumentsExecutionMode())); | |||
| childDispatch.setTailStatus(TailStatus.NOT_TAIL); | |||
| childDispatch.setTailStatus(getTailStatus()); | |||
There was a problem hiding this comment.
In the test below... what node behavior this change affects?
The warning in the test is attached to an argument, not to self, so I don't see how this change can be triggered and make a difference?
There was a problem hiding this comment.
Oh you mean why I change it in InvokeCallableNode when the fix was only necessary in InvokeMethodNode, right?
I'm reverting the previous change for all three cases and replacing it with the same approach i.e. warnings propagation via TailCallException.
It does not appear that we have any test case exercising this case specifically.
| Warning[] warnings, | ||
| @Cached(value = "createLoopNode()") LoopNode loopNode) { | ||
| Object result = dispatch(function, callerInfo, state, arguments, loopNode); | ||
| return WithWarnings.appendTo(EnsoContext.get(this), result, warnings); |
There was a problem hiding this comment.
This looks correct. If there are warnings, remove them, handle all the tail calls and attach them again. Not sure how that can be triggered by the test however?
There was a problem hiding this comment.
Not sure how that can be triggered by the test however?
I'm not sure how to convince you other than running it with debugger attached and putting the breakpoint yourself.
| extracted = warnings.getWarnings(warning, null); | ||
| callable = warnings.removeWarnings(warning); | ||
| } catch (UnsupportedMessageException e) { | ||
| throw CompilerDirectives.shouldNotReachHere(e); |
There was a problem hiding this comment.
Just a side note: Eventually, we should probably align the exceptions we are throwing from the nodes in the engine. In most places it is IllegalStateException, but sometimes it is also CompilerDirectives.shouldNotReachHere. We should provide a proper wrapper for these exceptions.
There was a problem hiding this comment.
Yeah, I've had the same observation a while ago ;)
Co-authored-by: Pavel Marek <[email protected]>
Pull Request Description
Partially revert #6849, which introduced a regression in TCO in the presence of warnings. Rather than modifying the tail call status,
TailCallExceptionnow 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.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
- [ ] The documentation has been updated, if necessary.- [ ] Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
- [ ] If GUI codebase was changed, the GUI was tested when built using./run ide build.