Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opt: Get rid of the LiftTry phase; instead handle things in the back-end. #18619

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Sep 29, 2023

When we enter a try-catch at the JVM level, we have to make sure that the stack is empty. That's because, upon exception, the JVM wipes the stack, and we must not lose operands that are already on the stack that we will still use.

Previously, this was achieved with a transformation phase, LiftTry, which lifted problematic try-catches in local defs, called liftedTree$x. It analyzed the tree to predict which try-catches would execute on a non-empty stack when eventually compiled to the JVM.

This approach has several shortcomings.

It exhibits performance cliffs, as the generated def can then cause more variables to be boxed in to XRefs. These were the only extra defs created for implementation reasons rather than for language reasons. As a user of the language, it is hard to predict when such a lifted def will be needed.

The additional liftedTree methods also show up on stack traces and obfuscate them. Debugging can be severely hampered as well.

Phases executing after LiftTry, notably CapturedVars, also had to take care not to create more problematic situations as a result of their transformations, which is hard to predict and to remember.

Finally, Scala.js and Scala Native do not have the same restriction, so they received suboptimal code for no reason.

In this commit, we entirely remove the LiftTry phase. Instead, we enhance the JVM back-end to deal with the situation. When starting a try-catch on a non-empty stack, we stash the entire contents of the stack into local variables. After the try-catch, we pop all those local variables back onto the stack. We also null out the leftover vars not to prevent garbage collection.

This new approach solves all of the problems mentioned above.


This could be back-ported to Scala 2 if there is interest.

/cc @adpi2 who wanted this to improve debugging.

@sjrd sjrd requested a review from lrytz September 29, 2023 11:55
@sjrd
Copy link
Member Author

sjrd commented Sep 29, 2023

@WojciechMazur Could you confirm that Scala Native indeed does not care about this?

@@ -372,10 +375,12 @@ trait BCodeBodyBuilder extends BCodeSkelBuilder {
// AbstractValidatingLambdaMetafactory.validateMetafactoryArgs

val DesugaredSelect(prefix, _) = fun: @unchecked
genLoad(prefix)
val prefixTK = genLoad(prefix)
stack.push(prefixTK)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing before, although it's a bit theoretical, since the arguments to Closures are always identifiers in practice, which means they don't contain any Return nor Try that would need an accurate stack.

…end.

When we enter a `try-catch` at the JVM level, we have to make sure
that the stack is empty. That's because, upon exception, the JVM
wipes the stack, and we must not lose operands that are already
on the stack that we will still use.

Previously, this was achieved with a transformation phase,
`LiftTry`, which lifted problematic `try-catch`es in local `def`s,
called `liftedTree$x`. It analyzed the tree to predict which
`try-catch`es would execute on a non-empty stack when eventually
compiled to the JVM.

This approach has several shortcomings.

It exhibits performance cliffs, as the generated def can then
cause more variables to be boxed in to `XRef`s. These were the
only extra defs created for implementation reasons rather than
for language reasons. As a user of the language, it is hard to
predict when such a lifted def will be needed.

The additional `liftedTree` methods also show up on stack traces
and obfuscate them. Debugging can be severely hampered as well.

Phases executing after `LiftTry`, notably `CapturedVars`, also had
to take care not to create more problematic situations as a result
of their transformations, which is hard to predict and to remember.

Finally, Scala.js and Scala Native do not have the same restriction,
so they received suboptimal code for no reason.

In this commit, we entirely remove the `LiftTry` phase. Instead, we
enhance the JVM back-end to deal with the situation. When starting
a `try-catch` on a non-empty stack, we stash the entire contents of
the stack into local variables. After the `try-catch`, we pop all
those local variables back onto the stack. We also null out the
leftover vars not to prevent garbage collection.

This new approach solves all of the problems mentioned above.
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me. I guess the overhead of saving and restoring the stack is OK because try blocks on a non-empty stack are not too common?

I was trying to think of ways this could break, i.e., is there a way that byecode within the try block could attempt to read something from the stack (which is now no longer there). I don't think so.

@sjrd
Copy link
Member Author

sjrd commented Oct 2, 2023

This seems good to me. I guess the overhead of saving and restoring the stack is OK because try blocks on a non-empty stack are not too common?

It is pretty rare, yes. But anyway, storing the stack in locals cannot be worse than calling an external method. Calling an external method would effectively have to stash the stack into local variables (or callee-saved registers, which is the same thing) at the JVM level.

I was trying to think of ways this could break, i.e., is there a way that byecode within the try block could attempt to read something from the stack (which is now no longer there). I don't think so.

I don't see how that could happen. genLoad and all other codegen functions (recursively) put on the stack everything they are going to consume. We can never consume things that are already on the stack.

@sjrd
Copy link
Member Author

sjrd commented Oct 2, 2023

@WojciechMazur
Copy link
Contributor

@WojciechMazur Could you confirm that Scala Native indeed does not care about this?

Yes, I can confirm we don't relay on LiftTry in ScalaNative and it can be removed

@sjrd sjrd merged commit 1bf0f6d into scala:main Oct 4, 2023
18 checks passed
@sjrd sjrd deleted the no-lifttry branch October 4, 2023 12:56
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jun 20, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
… the back-end." to LTS (#20638)

Backports #18619 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
… the back-end." to LTS (#20691)

Backports #18619 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants