Skip to content

Avoid ObjectRefs in two hot paths in compiler#8371

Merged
retronym merged 1 commit intoscala:2.12.xfrom
retronym:faster/object-ref
Aug 27, 2019
Merged

Avoid ObjectRefs in two hot paths in compiler#8371
retronym merged 1 commit intoscala:2.12.xfrom
retronym:faster/object-ref

Conversation

@retronym
Copy link
Member

Avoid capturing synthetic patmat vars in closures, etc, which
gives rise to an ObjectRef to box the var. There is currently a
bug in the pattern matcher that emits these more of these vars
under -optimize -- the logic was not updated woth -opt was
introduced in 2.12.

Rewrite RefChecks.transform in terms of vals to avoid another
ObjectRef.

I have experimented with fixing the disparity between
-opt and -optimize in retronym#65
but that's still WIP.

Avoid capturing synthetic patmat vars in closures, etc, which
gives rise to an ObjectRef to box the var. There is currently a
bug in the pattern matcher that emits these more of these vars
under `-optimize` -- the logic was not updated woth `-opt` was
introduced in 2.12.

Rewrite `RefChecks.transform` in terms of vals to avoid another
ObjectRef.
@scala-jenkins scala-jenkins added this to the 2.12.10 milestone Aug 26, 2019
@retronym retronym requested a review from hrhino August 26, 2019 05:14
// inside annotations.
applyRefchecksToAnnotations(tree)
var result: Tree = tree match {
val result: Tree = tree match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Renaming this variable result to result1 could make for a smaller diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and re-did the change with step-wise IDE refactorings starting with your suggestion and moving to the same end result as this PR to be sure I hadn't changed the meaning.

Copy link
Contributor

@hrhino hrhino left a comment

Choose a reason for hiding this comment

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

LGTM. I suppose the idea was to make debugging easier by giving names to the bound variables?

@retronym
Copy link
Member Author

retronym commented Aug 26, 2019

@hrhino I think so. See #1060.

   To facilitate debugging pattern matches, we store the values for
    sub-patterns of extractor (synthetic or user-defined) patterns in local variables.
    When performing an optimized build, and when possible, we don't do store but inline them directly.

    For soundness, SI-5158, SI-6070, we must always store the values of mutable case class fields.

    (Specifying -optimize is the only way to suppress emitting these local variables.
    An unoptimized build will always generate them, which was deemed the right default during the meeting.)

@retronym retronym merged commit d70893c into scala:2.12.x Aug 27, 2019
@retronym
Copy link
Member Author

It occurs to me that we could change the compiler to avoid boxing these synthetic vars in ObjectRefs given the knowledge that there are no assignments after the point of capture. I'm pretty sure this is true by construction for the pattern matcher code gen.

It appears to be a pretty straightforward change: retronym#66

@retronym
Copy link
Member Author

/cc @adriaanm

@SethTisue SethTisue added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance the need for speed. usually compiler performance, sometimes runtime performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants