Fix #17549: Unify how Memoize and Constructors decide what fields need storing. #17560
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjrd
commented
May 23, 2023
| @@ -2,6 +2,6 @@ T.f1 | |||
| T.f2 | |||
| T.f3 | |||
| T.f4 | |||
| 3 2 -3 -4 | |||
| 3 2 3 -4 | |||
There was a problem hiding this comment.
AFAICT, this is a progression.
There was a problem hiding this comment.
This seems dubious to me because the result leaks before the side-effect runs. Maybe that never matters because leaking a more correct value is always better, given the semantics of initialization. But I had to convince myself.
smarter
approved these changes
May 23, 2023
Comment on lines
264
to
287
| def isConstExprFinalVal(using Context): Boolean = | ||
| atPhaseNoLater(erasurePhase) { | ||
| self.is(Final) && self.info.resultType.isInstanceOf[ConstantType] | ||
| self.is(Final, butNot = Mutable) && self.info.resultType.isInstanceOf[ConstantType] | ||
| } && !self.sjsNeedsField | ||
|
|
||
| def constExprFinalValConstantType(using Context): ConstantType = | ||
| atPhaseNoLater(erasurePhase) { | ||
| self.info.resultType.asInstanceOf[ConstantType] | ||
| } |
There was a problem hiding this comment.
This looks like it could be expressed in a cleaner way using an extractor object?
There was a problem hiding this comment.
I don't think so. There is only 1 call site out of 5 that would benefit from the extractor variant. The other 4 would regress in readability.
There was a problem hiding this comment.
ok, can you add a comment to constExprFinalValConstantType with something like @pre self.isConstExprFinalVal to make the pre-condition explicit?
…s need storing. The Memoize and Constructors have to work together and agree on which `final val`s actually need storing in a field. Previously, they used slightly different criteria: one on the result type, and one on the rhs (with an additional Scala.js-specific eligibility condition). That discrepancy resulted in the crash/wrong codegen in the issue. We now unify both: we avoid a field if and only if all of the following apply: * it is a `final val`, * its result *type* is a `ConstantType`, and * it is eligible according to Scala.js semantics. In particular, there is no condition on the right-hand-side. We avoid a field even if the right-hand-side has side effects. The side effects are moved to the constructor regardless. --- This introduces both progressions and regressions in the amount of fields we generate. We can avoid fields even for side-effecting rhs'es, as long as the result type is constant. On the other hand, we now create a field for `final val`s with non-constant result type, even if the rhs is a literal. While the latter is a regression for Scala 3, it aligns with the behavior of Scala 2. It also has the nice benefit that whether or not a val has a field is now independent of its *implementation*, and only dependent on its *API*. Overall, I think this is a trade-off worth taking. We could reintroduce that optimization in the future (but in classes only; not in traits), if we really want to, although that would require dedicated code.
sjrd
force-pushed
the
fix-codegen-final-vals
branch
from
dd0121d
to
ac2b53b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Memoize and Constructors have to work together and agree on which
final vals actually need storing in a field. Previously, they used slightly different criteria: one on the result type, and one on the rhs (with an additional Scala.js-specific eligibility condition). That discrepancy resulted in the crash/wrong codegen in the issue.We now unify both: we avoid a field if and only if all of the following apply:
final val,ConstantType, andIn particular, there is no condition on the right-hand-side. We avoid a field even if the right-hand-side has side effects. The side effects are moved to the constructor regardless.
This introduces both progressions and regressions in the amount of fields we generate. We can avoid fields even for side-effecting rhs'es, as long as the result type is constant. On the other hand, we now create a field for
final vals with non-constant result type, even if the rhs is a literal.While the latter is a regression for Scala 3, it aligns with the behavior of Scala 2. It also has the nice benefit that whether or not a val has a field is now independent of its implementation, and only dependent on its API. Overall, I think this is a trade-off worth taking.
We could reintroduce that optimization in the future (but in classes only; not in traits), if we really want to, although that would require dedicated code.