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.

Sign up for GitHub

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

Jump to bottom

SI-4859 Step back from mis-optimizations in qualifiers #1864

Merged
merged 6 commits into from Jan 26, 2013
Merged

SI-4859 Step back from mis-optimizations in qualifiers #1864

merged 6 commits into from Jan 26, 2013

Conversation

retronym
Copy link
Member

@retronym retronym commented Jan 8, 2013

Retargetting #1710 to master.

Widespread, invalid qualifier discarding in the apparent name of performance led to a variety of problems:

The compiler would crash in case of:

OuterCaseClass().InnerCaseClass()

And the side effect would be elided in:

{ println("!!"); Outer}.Inner

This change fixes these problems but stops short of fixing the last problem:

Outer.Inner.foo // module load of Outer is still elided.

This could be fixed use a stronger condition than isExprSafeToInline throughout this patch. (It would be isExprSafeToElide or isPure.)

Thoughts? Should that change be tackled in this ticket or separately?

Last time, @paulp opined:

I don't see how we can fail to initialize Outer here if we wish to have at all predictable semantics.

Review by @paulp @magarcia

@ghost ghost assigned paulp Jan 8, 2013
@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1330/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1330/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2044/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2044/

@magarciaEPFL
Copy link
Contributor

As a sidenote, the experimental optimizer detects bytecode-level repeated reads of stable accessors, caching in a synthetic variable upon first access and reusing it afterwards (once per distinct control-flow path, which works pretty well for common cases).

@adriaanm
Copy link
Contributor

/localhome/jenkins/a/workspace/pr-scala-testsuite-linux-opt/test/files/run/t0091.scala [FAILED]

@paulp
Copy link
Contributor

paulp commented Jan 15, 2013

This looks good to me if the verifier errors are gone. I think the idea of optimizer decisions being configurable at user level will also be necessary for a satisfying resolution to these inlining questions. A strict view that no behavioral change can take place rules out way too much; but how and where to relax the grip is another one-size-only-fits-some situations.

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2140/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2140/

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1415/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1415/

@retronym
Copy link
Member Author

Rebased against master.

@paulp
Copy link
Contributor

paulp commented Jan 17, 2013

Eez good, mistah retronym.

@paulp
Copy link
Contributor

paulp commented Jan 17, 2013

But I guess somebody better run the tests since the kitteh is too good for us. I'll give them a whirl.

@scala-jenkins
Copy link

Started jenkins job JenkinsJob(pr-rangepos) at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1488/

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1573/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2300/

@scala-jenkins
Copy link

Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1593/

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2318/

@scala-jenkins
Copy link

jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1593/
sad kitty

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2318/
sad kitty

@retronym
SI-4859 Don't rewrite CC().CC2() to new CC2
61f2936 
Where CC and CC2 are case classes. Attempting to do so leads to
a "no legal prefix" error.

Now, we restrict this optimization (living in RefChecks ?!) to
case class applies with a "safe to inline" qualifier.
@retronym
Introduce a new Symbol test: isTopLevel.
3813d75
@retronym
Wider use of isTopLevel
eb4b065
@retronym
SI-4859 Don't elide qualifiers when selecting nested modules.
f21b1ce 
Otherwise we fail to throw in:

   {???; Predef}.DummyImplicit.dummyImplicit

We still elide the initialization of `Outer` in `Outer.Inner.foo`
as before, although that seems a little dubious to me.

In total, we had to change RefChecks, Flatten, and GenICode
to effect this change. A recently fixed bug in tail call elimination
was also due to assuming that the the qualifier of a Select node
wasn't worthy of traversal.  Let's keep a close eye out for more
instances of this problem.
@retronym
SI-4859 Retain MODULE_LOAD in dead code elim.
412ad57 
Without this, the following test fails:

    SCALAC_OPTS="-optimize" ./test/partest test/files/run/t4859.scala
@retronym
Laying groundwork for a followup ticket.
2580a51 
To solve SI-5304, we should change `isQualifierSafeToElide`.
@retronym
Copy link
Member Author

PLS REBUILD ALL

While we wait, Martin's viewpoint on whether A.B.C.foo should load modules A and B (reminder: it still doesn't under this patch):

  • Accessing a nested Java static classes doesn't run the static initializer of the enclosing class
  • People might be surprised by a performance difference between nested and top level modules
  • But, the arguments for forcing initialization do make sense.
  • We might investigate a scheme in which the constructor of a nested module initializes it's enclosing module. This would give us the desired semantics without the penalty of the outer module load at every access of the nested module.

I'll add these comments to SI-5304.

@paulp
Copy link
Contributor

paulp commented Jan 26, 2013

From the standpoint of performance, I think we should be a lot more worried about the impact the loading semantics have on the potential for optimization: ours, proguard's, and hotspot's. All of us have to allow for module loading side effects, because we offer no true statics. I'm not sure how large is the cumulative effect, but it rears its head in enough places to add up.

@paulp
Copy link
Contributor

paulp commented Jan 26, 2013

Since we're still waiting for a peep, i started a manual test run. https://scala-webapps.epfl.ch/jenkins/job/scala-checkin-manual/769/console

@paulp
Copy link
Contributor

paulp commented Jan 26, 2013

Yay, the tests passed. I'll fix the merge.

paulp added a commit that referenced this pull request Jan 26, 2013
@paulp
Merge pull request #1864 from retronym/ticket/4859-6
a24fd59 
SI-4859 Step back from mis-optimizations in qualifiers
@paulp paulp merged commit 2580a51 into scala:master Jan 26, 2013
@scabug scabug mentioned this pull request Apr 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@paulp paulp

Labels
None yet
Projects
None yet
Milestone
2.11.0-M2
5 participants
@retronym @scala-jenkins @magarciaEPFL @adriaanm @paulp