New lazy vals implementation #15296

szymon-rd · 2022-05-26T12:55:24Z

Based on #14545

Implementing the new lazy vals scheme mentioned in #6979, fixing #7140

New PR to avoid force pushing to main branch of the previous contributor.

Fixes #15149 as well
Applied #14780

szymon-rd · 2022-05-26T12:56:48Z

@odersky Created this as a new PR with source branch at dotty-staging so you can edit it.

odersky

It looks overall good to me but I was not able to check details for lack of time. Can someone else give it a thorough go-over? If this is going to be the new implementation of LazyVals, it would be good to get one more person to know in detail what it does.

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

sjrd

Overall, this looks good. I have some suggestions here and there.

What would it take to leave the old implementation available under a Java system property? In the past in Scala 2, performance-risky changes like this had a fallback on the old implementation that could be enabled with a Java system property (or a compiler flag). I think this is a situation that would benefit from such an escape hatch.

library/src/scala/runtime/LazyVals.scala

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

library/src/scala/runtime/LazyVals.scala

prolativ · 2022-06-01T11:33:42Z

test performance please

bishabosha · 2022-06-01T11:36:54Z

test performance please

dottybot · 2022-06-01T11:37:52Z

performance test scheduled: 7 job(s) in queue, 1 running.

dottybot · 2022-06-01T13:17:26Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15296/ to see the changes.

Benchmarks is based on merging with main (89c3c10)

szymon-rd · 2022-06-01T13:31:27Z

test performance please

dottybot · 2022-06-01T13:31:46Z

performance test scheduled: 6 job(s) in queue, 1 running.

allanrenucci · 2022-06-01T14:31:31Z

A few years ago, I wrote micro-benchmarks for Dotty lazy val implementations: https://github.com/allanrenucci/dotty-benchmark/tree/lazy-vals-v2

You may be interested in running these benchmarks against the new implementation.

szymon-rd · 2022-06-01T14:47:10Z

@allanrenucci thanks, I will use it

dottybot · 2022-06-01T17:19:41Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15296/ to see the changes.

Benchmarks is based on merging with main (3ddb21c)

smarter · 2022-06-01T17:26:03Z

library/src/scala/runtime/LazyVals.scala

@@ -43,22 +45,21 @@ object LazyVals {
   * evaluated and of which other threads await the result.
   */
  final class Waiting:
-    private var done = false
+    private val countDownLatch: CountDownLatch = new CountDownLatch(1)


Any reason to use a field instead of extends CountDownLatch(1)? The field adds one extra level of pointer-chasing.

szymon-rd · 2022-06-06T14:41:21Z

test performance please

dottybot · 2022-06-06T14:42:43Z

performance test scheduled: 1 job(s) in queue, 1 running.

dottybot · 2022-06-06T17:20:54Z

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15296/ to see the changes.

Benchmarks is based on merging with main (e71fe80)

szymon-rd · 2022-06-08T10:00:22Z

@allanrenucc I think that some files may be missing on this branch - LazyVolatile and LazySimCellWithPublicBitmap are not there. Do you happen to know where to find them?

allanrenucci · 2022-06-08T13:58:02Z

I don't fully remember what these objects refer to. LazyVolatile looks very similar to scala.runtime.LazyVals. Not sure about LazySimCellWithPublicBitmap .

szymon-rd · 2022-10-21T15:51:31Z

@sjrd I responded to / applied all of your comments. Could you take a look?

sjrd

Looks great, thanks! I only have very minor comment-related comments left.

Could you also rebase on main so that there is no merge commit appearing in the PR? In fact, it looks like it would be better to squash all the commits of this PR anyway, at this point.

sjrd · 2022-10-27T12:59:25Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+   *   else if result.eq(NullValue) then
+   *     null // possible unboxing applied here
+   *   else
+   *     return x_compute() // possible unboxing applied here


Suggested change

* return x_compute() // possible unboxing applied here

* x_compute() // possible unboxing applied here

sjrd · 2022-10-27T13:01:10Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+   *   else
+   *     return x_compute() // possible unboxing applied here
+   * 
+   * private def x_compute() =


Suggested change

* private def x_compute() =

* private def x_compute(): AnyRef =

so that we don't have to guess?

sjrd · 2022-10-27T13:02:22Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+   *           current.asInstanceOf[Waiting].await()
+   *         else return null
+   *       else
+   *         return current.asInstanceOf[A]


Suggested change

* return current.asInstanceOf[A]

* return current

sjrd · 2022-10-27T13:06:01Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+        Block(ValDef(resSymb, nullLiteral) :: ValDef(resSymbNullable, nullLiteral) :: evaluate :: Nil, // var result: AnyRef = null
+          Return(ref(resSymbNullable), lazyInitMethodSymbol)), // try ... finally ...


The comments on the right do not match up anymore.

Pre-review lazy vals adjustments

szymon-rd · 2022-10-28T14:04:05Z

@sjrd I applied your suggestions, squashed some of the commits (I kept the most important "pieces" but squashed all the smaller adjustments). I also rebased on the main.

SethTisue · 2022-10-28T21:34:39Z

Congrats on landing this! 🎉

Curious... is the PR description for #6979 still the best place for users to read about what this change is and what implications it might have for their libraries and applications? (Or are we confident there aren't any implications for users?)

szymon-rd requested a review from odersky May 26, 2022 12:55

szymon-rd mentioned this pull request May 26, 2022

New lazy vals implementation #15207

Closed

szymon-rd self-assigned this May 26, 2022

odersky reviewed May 26, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/transform/LazyVals.scala Outdated Show resolved Hide resolved

Kordyjan requested review from sjrd and OlivierBlanvillain May 26, 2022 15:58

Kordyjan assigned sjrd and unassigned szymon-rd May 26, 2022

sjrd requested changes May 30, 2022

View reviewed changes

szymon-rd added this to the 3.2.0-RC1 milestone May 30, 2022

TheElectronWill mentioned this pull request May 31, 2022

GraalVM Native Image Requires Config for lazy vals #13985

Closed

bishabosha reviewed May 31, 2022

View reviewed changes

library/src/scala/runtime/LazyVals.scala Outdated Show resolved Hide resolved

smarter reviewed Jun 1, 2022

View reviewed changes

szymon-rd force-pushed the new-lazy-vals branch 2 times, most recently from e94d78c to 71c7747 Compare June 6, 2022 14:23

szymon-rd force-pushed the new-lazy-vals branch from 4cbce0d to dbdf38f Compare October 21, 2022 15:00

szymon-rd requested a review from sjrd October 21, 2022 15:51

szymon-rd force-pushed the new-lazy-vals branch from dbdf38f to 28abd42 Compare October 22, 2022 10:21

sjrd requested changes Oct 27, 2022

View reviewed changes

szymon-rd force-pushed the new-lazy-vals branch 5 times, most recently from dedf464 to c791c55 Compare October 28, 2022 13:40

olsdavis and others added 6 commits October 28, 2022 15:42

New lazy vals implementation

b9d5565

New version of Lazy Vals implementation

3ef3a19

Benchmarking lazy vals

5442089

Optimizing performance of LazyVals

81094be

Fix failing testCompilation tests for new lazy vals

937424a

Pre-review lazy vals adjustments

Revert scala-parallel-collection community build to main branch

572c674

szymon-rd force-pushed the new-lazy-vals branch from c791c55 to 955c0be Compare October 28, 2022 13:45

Clean-up the new lazy vals

f62ffd8

szymon-rd force-pushed the new-lazy-vals branch from 955c0be to f62ffd8 Compare October 28, 2022 14:03

sjrd approved these changes Oct 28, 2022

View reviewed changes

szymon-rd merged commit 5bd67bb into scala:main Oct 28, 2022

szymon-rd deleted the new-lazy-vals branch October 28, 2022 15:58

SethTisue mentioned this pull request Oct 31, 2022

new lazy val locking scheme scala/scala-dev#376

Open

szymon-rd mentioned this pull request Dec 19, 2022

Stabilize new lazy vals #16555

Closed

Kordyjan modified the milestones: 3.3.0-RC1, 3.2.2 Aug 1, 2023

SethTisue mentioned this pull request May 3, 2024

Runtime code implementing lazy val should not use sun.misc.Unsafe on Java 9+ (JEP-471) #9013

Open

WojciechMazur mentioned this pull request Jun 26, 2024

Outdated LazyVals reference in documentaiton #20820

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lazy vals implementation #15296

New lazy vals implementation #15296

szymon-rd commented May 26, 2022 •

edited

Loading

szymon-rd commented May 26, 2022 •

edited

Loading

odersky left a comment

sjrd left a comment

prolativ commented Jun 1, 2022

bishabosha commented Jun 1, 2022

dottybot commented Jun 1, 2022

dottybot commented Jun 1, 2022

szymon-rd commented Jun 1, 2022

dottybot commented Jun 1, 2022

allanrenucci commented Jun 1, 2022

szymon-rd commented Jun 1, 2022

dottybot commented Jun 1, 2022

smarter Jun 1, 2022

szymon-rd commented Jun 6, 2022

dottybot commented Jun 6, 2022

dottybot commented Jun 6, 2022

szymon-rd commented Jun 8, 2022 •

edited

Loading

allanrenucci commented Jun 8, 2022

szymon-rd commented Oct 21, 2022

sjrd left a comment

sjrd Oct 27, 2022

sjrd Oct 27, 2022

sjrd Oct 27, 2022

sjrd Oct 27, 2022

szymon-rd commented Oct 28, 2022

SethTisue commented Oct 28, 2022 •

edited

Loading

	* return x_compute() // possible unboxing applied here
	* x_compute() // possible unboxing applied here

	* private def x_compute() =
	* private def x_compute(): AnyRef =

		Block(ValDef(resSymb, nullLiteral) :: ValDef(resSymbNullable, nullLiteral) :: evaluate :: Nil, // var result: AnyRef = null
		Return(ref(resSymbNullable), lazyInitMethodSymbol)), // try ... finally ...

New lazy vals implementation #15296

New lazy vals implementation #15296

Conversation

szymon-rd commented May 26, 2022 • edited Loading

szymon-rd commented May 26, 2022 • edited Loading

odersky left a comment

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

prolativ commented Jun 1, 2022

bishabosha commented Jun 1, 2022

dottybot commented Jun 1, 2022

dottybot commented Jun 1, 2022

szymon-rd commented Jun 1, 2022

dottybot commented Jun 1, 2022

allanrenucci commented Jun 1, 2022

szymon-rd commented Jun 1, 2022

dottybot commented Jun 1, 2022

smarter Jun 1, 2022

Choose a reason for hiding this comment

szymon-rd commented Jun 6, 2022

dottybot commented Jun 6, 2022

dottybot commented Jun 6, 2022

szymon-rd commented Jun 8, 2022 • edited Loading

allanrenucci commented Jun 8, 2022

szymon-rd commented Oct 21, 2022

sjrd left a comment

Choose a reason for hiding this comment

sjrd Oct 27, 2022

Choose a reason for hiding this comment

sjrd Oct 27, 2022

Choose a reason for hiding this comment

sjrd Oct 27, 2022

Choose a reason for hiding this comment

sjrd Oct 27, 2022

Choose a reason for hiding this comment

szymon-rd commented Oct 28, 2022

SethTisue commented Oct 28, 2022 • edited Loading

szymon-rd commented May 26, 2022 •

edited

Loading

szymon-rd commented May 26, 2022 •

edited

Loading

szymon-rd commented Jun 8, 2022 •

edited

Loading

SethTisue commented Oct 28, 2022 •

edited

Loading