scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Case class rewrite affects order of construction/evaluation/allocation #12903

Closed som-snytt closed 7 months ago

som-snytt commented 7 months ago

Reproduction steps

Scala version: 2.13, 3.x

➜  scala -Dscala.repl.info -J--add-exports -Jjdk.jdeps/com.sun.tools.javap=ALL-UNNAMED -nobootcp -cp /tmp/sandbox 
[snip]

scala 2.13.12> :pa -raw
// Entering paste mode (ctrl-D to finish)

case class C(x: Int)

// Exiting paste mode, now interpreting.

scala 2.13.12> def f(x: => Int) = C(x)
def f(x: => Int): C

scala 2.13.12> :javap f
[snip]
  public C f(scala.Function0<java.lang.Object>);
    descriptor: (Lscala/Function0;)LC;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=2, args_size=2
         0: new           #18                 // class C
         3: dup
         4: aload_1
         5: invokeinterface #24,  1           // InterfaceMethod scala/Function0.apply$mcI$sp:()I
        10: invokespecial #28                 // Method C."<init>":(I)V
        13: areturn

Problem

The by-name arg is evaluated after allocating (new). This is different from evaluating the arg before calling apply.

Strictly, the rewrite should pre-compute the arg to a temporary, since the computation may fail.

This doesn't matter much, but was noticed at https://github.com/scala/scala/pull/10600

Dotty constructor proxies work the same way.

SethTisue commented 7 months ago

since the computation may fail

and what are the consequences in that case?

som-snytt commented 7 months ago

It's an unnecessary allocation. People wonder in the context, "Can I save an allocation here?" as on the linked PR.

I'm looking at the mechanics of allocation on the jvm, where init is not completed.

lrytz commented 7 months ago

I believe I heard that they are going to eliminate the new; dup; load args; invokespecial <init> dance in a future Java release, but cannot find a reference right now.

In general, I don't think we need to optimize for this. If it was a measurable issue we would probably have heard about it by now. Also there's the "Java does the same" argument hammer.