Emit local module like lazy val by adriaanm · Pull Request #5430 · scala/scala

adriaanm · 2016-09-28T17:39:44Z

The motivation is to use the new fine-grained lock scoping that
local lazies have since #5294.

Fixes scala/scala-dev#235

Co-Authored-By: Jason Zaugg jzaugg@gmail.com

Review by @adriaanm

TODO:

the outer pointer for the object after this PR is null:
(C$O$2$)O$lzy$1.initialize((Object)new C$O$2$(null)); versus x$1.elem = new C$O$2$(this);
the name for the local variable changed O$module -> O$lzy

Follow up for #5428

There's still a lot of duplication, as well as plenty of opportunities for constant folding / simplification.

adriaanm · 2016-09-28T17:41:28Z

I couldn't help but make some constants final in d04cda1 when I was fixing the name of the val in the expansion. @retronym, @lrytz: any reservations about this?

The outer pointer being null is progress, actually, since delambdafy correctly elides it. I added a test case where it isn't elided because it's used.

adriaanm · 2016-09-28T22:55:52Z

/rebuild

Hrm. Is the ec2 instance type not the issue here?

SethTisue · 2016-09-28T23:02:18Z

# starting 63 tests in run
!!  1 - run/SI-4676.scala                         [compilation failed]
!!  2 - run/SI-4360.scala                         [compilation failed]
!!  3 - run/SI-4887.scala                         [compilation failed]
##### Log file '/home/jenkins/workspace/scala-2.12.0-validate-test/test/scaladoc/run/SI-4360-run.log' from failed test #####

error: Java heap space

##### Log file '/home/jenkins/workspace/scala-2.12.0-validate-test/test/scaladoc/run/SI-4887-run.log' from failed test #####

error: Java heap space

##### Log file '/home/jenkins/workspace/scala-2.12.0-validate-test/test/scaladoc/run/SI-4676-run.log' from failed test #####

error: Java heap space

adriaanm · 2016-09-28T23:06:03Z

Right, so that error occurred on the behemoth that had already been upgraded to 4xlarge...

adriaanm · 2016-09-28T23:06:23Z

How come the max heap space is 1024m in that log?

Java options are: -Xmx1024M -Xms64M -XX:MaxPermSize=128M

I thought we had it at 2g?

SethTisue · 2016-09-28T23:09:20Z

git grep Xmx shows all sorts of different numbers in different places :-\

SethTisue · 2016-09-28T23:10:19Z

even more numbers if you do the same git grep in the scala-jenkins-infra, but in a partest context, I think it's the scala/scala numbers that matters since it's forked JVMs

adriaanm · 2016-09-28T23:10:36Z

build.sbt:635: testOptions in IntegrationTest += Tests.Argument("-Dpartest.java_opts=-Xmx1024M -Xms64M -XX:MaxPermSize=128M"),

retronym · 2016-09-28T23:39:43Z

consting those strings seems okay to me.

retronym · 2016-09-28T23:42:46Z

build.sbt

    testFrameworks += new TestFramework("scala.tools.partest.sbt.Framework"),
    testFrameworks -= new TestFramework("org.scalacheck.ScalaCheckFramework"),
-    testOptions in IntegrationTest += Tests.Argument("-Dpartest.java_opts=-Xmx1024M -Xms64M -XX:MaxPermSize=128M"),
+    testOptions in IntegrationTest += Tests.Argument(s"-Dpartest.java_opts=${(javaOptions in IntegrationTest).value.mkString(" ")}"),


Using a small initial heap for all the forked partest processes seems desirable to me, this change drops that:

show test/it:javaOptions [info] List(-Xmx2G)

If we actually have partests that require large heaps, we should probably segregated them into a new category that is run sequentially. That would let us run the vast majority of them with a small heap.

I like the idea of a large heap section.

Regarding the initial heap size, after this PR it would be 472M according to java -Xmx2g -XX:+PrintFlagsFinal. Should I move the -Xms64m setting to javaOptions in IntegrationTest += "-Xmx2G",?

Yep, the JVM that forks for the partest framework itself can probably get away with a small heap, too. I have a feeling that we bumped up the flags of that JVM to try to get around intermittent failures that were actually fixed later on this line.

Happy to do whatever gets things working again for 2.12.0 and leave the cleanup work until 2.12.x; I've noted my ideas in scala/scala-jenkins-infra#181 (comment)

retronym · 2016-09-29T02:29:08Z

Still failing. Very hard from the output to see which JVM is actually heap bound.

Should we add -XX:+CrashOnOutOfMemoryError to all our JVMs to fail fast with a crash log to help diagnosis?

adriaanm · 2016-09-29T03:18:22Z

Added it to the tests, for a start. No idea what's going on here. This is running on an EC2 c4.4xlarge instance, which has 30G ram.

adriaanm · 2016-09-29T04:04:50Z

We'll have to wait until we can upgrade to a b92 or later jdk for the crash on out of memory option.

lrytz

LGTM, also the name constants.

I don't understand what you're referring to by "outer pointer being null is progress / added a test case"

lrytz · 2016-09-29T12:40:24Z

src/compiler/scala/tools/nsc/transform/Fields.scala

      * ```
+      *
+      * The expansion is the same for local lazy vals and local objects,
+      * except for the name of the val ($lzy or


or? $module?

darn, yes, or $module :-)

adriaanm · 2016-09-29T14:38:44Z

"outer pointer being null is progress / added a test case"

I was surprised by the diff in generated code for a local test I was doing, but forgot you can't see my screen, so yeah, that was a mysterious comment 😎 The test looked something like class C { def foo = object O }.

In that test, the outer pointer passed to the local module's class's constructor is now null, instead of C.this. It turns out delambdafy now correctly detects it's not being used.

The motivation is to use the new fine-grained lock scoping that local lazies have since scala#5294. Fixes scala/scala-dev#235 Co-Authored-By: Jason Zaugg <jzaugg@gmail.com>

A local lazy val and a local object are expanded in the same way.

adriaanm · 2016-09-29T18:57:55Z

I dropped all the CI-related stuff (moved discussion to scala/scala-dev#236) and fixed the doc

Make some name suffixes constants

d04cda1

There's still a lot of duplication, as well as plenty of opportunities for constant folding / simplification.

scala-jenkins added this to the 2.12.0-RC2 milestone Sep 28, 2016

adriaanm force-pushed the dev235 branch from 346b012 to b2a9726 Compare September 28, 2016 21:10

adriaanm force-pushed the dev235 branch from b2a9726 to 0e9fc2f Compare September 28, 2016 23:17

retronym reviewed Sep 28, 2016

View reviewed changes

adriaanm changed the title ~~Emit local module like lazy val~~ Emit local module like lazy val [ci: last-only] Sep 29, 2016

adriaanm force-pushed the dev235 branch 2 times, most recently from e025648 to 9c738a1 Compare September 29, 2016 01:28

retronym mentioned this pull request Sep 29, 2016

JVM crash with "There is insufficient memory for the Java Runtime Environment to continue" scala/scala-jenkins-infra#181

Closed

adriaanm force-pushed the dev235 branch from 9c738a1 to 325fffe Compare September 29, 2016 03:17

adriaanm force-pushed the dev235 branch from 325fffe to 067fcc6 Compare September 29, 2016 04:04

adriaanm force-pushed the dev235 branch from 067fcc6 to 76b7158 Compare September 29, 2016 05:29

lrytz reviewed Sep 29, 2016

View reviewed changes

adriaanm and others added 2 commits September 29, 2016 11:49

Emit local module like lazy val

493e22f

The motivation is to use the new fine-grained lock scoping that local lazies have since scala#5294. Fixes scala/scala-dev#235 Co-Authored-By: Jason Zaugg <jzaugg@gmail.com>

Clarify docs, variable name

515bc60

A local lazy val and a local object are expanded in the same way.

adriaanm force-pushed the dev235 branch from 76b7158 to 515bc60 Compare September 29, 2016 18:57

adriaanm changed the title ~~Emit local module like lazy val [ci: last-only]~~ Emit local module like lazy val Sep 29, 2016

adriaanm merged commit 1615ce9 into scala:2.12.0 Sep 29, 2016

lrytz mentioned this pull request Sep 30, 2016

Should method local modules lock on modules use the new finer grained locks of local lazy vals? scala/scala-dev#235

Closed

adriaanm added the release-notes worth highlighting in next release notes label Oct 15, 2016

adriaanm modified the milestone: 2.12.0-RC2 Oct 29, 2016

adriaanm added the 2.12 label Oct 29, 2016

Conversation

adriaanm commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Sep 28, 2016

Uh oh!

adriaanm commented Sep 28, 2016

Uh oh!

SethTisue commented Sep 28, 2016

Uh oh!

adriaanm commented Sep 28, 2016

Uh oh!

adriaanm commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Sep 28, 2016

Uh oh!

SethTisue commented Sep 28, 2016

Uh oh!

adriaanm commented Sep 28, 2016

Uh oh!

retronym commented Sep 28, 2016

Uh oh!

retronym Sep 28, 2016

Choose a reason for hiding this comment

Uh oh!

retronym Sep 28, 2016

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 29, 2016

Choose a reason for hiding this comment

Uh oh!

retronym Sep 29, 2016

Choose a reason for hiding this comment

Uh oh!

retronym commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Sep 29, 2016

Uh oh!

adriaanm commented Sep 29, 2016

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

lrytz Sep 29, 2016

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 29, 2016

Choose a reason for hiding this comment

Uh oh!

adriaanm Sep 29, 2016

Choose a reason for hiding this comment

Uh oh!

adriaanm commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adriaanm commented Sep 28, 2016 •

edited

Loading

adriaanm commented Sep 28, 2016 •

edited

Loading

retronym commented Sep 29, 2016 •

edited

Loading

adriaanm commented Sep 29, 2016 •

edited

Loading

adriaanm commented Sep 29, 2016 •

edited

Loading