Closes #23681: Unnecessary reference duplicate in backend

[guillaumecarraux]

from #23681:

Compiler version

Scala 3.8.0-RC1-bin-20250731-fe6b7eb-NIGHTLY, JVM (21)

Minimized code

//> using scala 3.nightly

def spin = while  true do {}

class A

@main def main =
  //creating a large object
  var large: Array[Double] | Null = Array.fill(10_000_000)(1.0)
  val other = A()

  //comparing it with another object (on the rhs)
  println((other == large))

  //dropping the reference to the large object
  large = null

  //spin to check memory usage
  spin

Output

The memory grabbed by the large object is not freed, even though there is no variable holding it.

Root cause

I suspect this is because of the current backend behaviour when generating the byte code for an == operation (in BCodeBodyBuilder.scala:genEqEqPrimitive):

val eqEqTempLocal = locals.makeLocal(ObjectRef, nme.EQEQ_LOCAL_VAR.mangledString, defn.ObjectType, r.span)

It currently creates a copy of the right-hand side of the "==", so that it can be used later without reevaluating it. However, this copy can create a new GC root, and is a potential memory leak, invisible to the user.

Suggested fix

I suggest to replace this local variable copy by some stack operations, which might also be slightly faster.

current backend code:

val eqEqTempLocal = locals.makeLocal(ObjectRef, nme.EQEQ_LOCAL_VAR.mangledString, defn.ObjectType, r.span)
val lNull    = new asm.Label
val lNonNull = new asm.Label

genLoad(l, ObjectRef) //  load lhs
stack.push(ObjectRef)
genLoad(r, ObjectRef) // load rhs
stack.pop()
locals.store(eqEqTempLocal) // store rhs in a local variable
bc dup ObjectRef // duplicate top stack variable (lhs)
genCZJUMP(lNull, lNonNull, Primitives.EQ, ObjectRef, targetIfNoJump = lNull) // compare lhs with NULL

markProgramPoint(lNull)
bc drop ObjectRef // drop top stack variable (lhs)
locals.load(eqEqTempLocal) // load rhs then compare with NULL
genCZJUMP(success, failure, Primitives.EQ, ObjectRef, targetIfNoJump = lNonNull)

markProgramPoint(lNonNull)
locals.load(eqEqTempLocal) // load rhs then compare with lhs
genCallMethod(defn.Any_equals, InvokeStyle.Virtual)
genCZJUMP(success, failure, Primitives.NE, BOOL, targetIfNoJump)

I suggest replacing with the following simple stack operations, with the resulting operand stack in comment:

-          val eqEqTempLocal = locals.makeLocal(ObjectRef, nme.EQEQ_LOCAL_VAR.mangledString, defn.ObjectType, r.span)
           val lNull    = new asm.Label
           val lNonNull = new asm.Label
 
-          genLoad(l, ObjectRef)
+          genLoad(r, ObjectRef) //  load rhs --> (r)
           stack.push(ObjectRef)
-          genLoad(r, ObjectRef)
+          genLoad(l, ObjectRef) // load lhs --> (l,r)
           stack.pop()
-          locals.store(eqEqTempLocal)
           bc dup ObjectRef // duplicate top stack variable --> (l,l,r)
           genCZJUMP(lNull, lNonNull, Primitives.EQ, ObjectRef, targetIfNoJump = lNull) // compare lhs with NULL --> (l,r)
 
           markProgramPoint(lNull)
           bc drop ObjectRef // drop top stack variable --> (r)
-          locals.load(eqEqTempLocal)
           genCZJUMP(success, failure, Primitives.EQ, ObjectRef, targetIfNoJump = lNonNull) // --> (-)
 
           markProgramPoint(lNonNull)
-          locals.load(eqEqTempLocal)
+          emit(asm.Opcodes.SWAP) //swap l and r for correct .equals ordering --> (r,l)
           genCallMethod(defn.Any_equals, InvokeStyle.Virtual) // --> (-)
           genCZJUMP(success, failure, Primitives.NE, BOOL, targetIfNoJump)

The test DottyBytecodeTests:patmatControlFlow is modified to match the new bytecode.

[sjrd]

We were actually considering generating a call to java.util.Objects.equals a few weeks ago. I believe that would also solve this issue, while (hopefully?) produce shorter bytecode.

/cc @lrytz

[lrytz]

That would be nice. The motivation we brought it up is code coverage tools, the null branch is typically unused. cc @retronym

The Objects.equals call needs to be inlined by the JVM, as the call to Object.equals inside is megamorphic. I assume if that doesn't happen reliably we would find out pretty quickly, given how widespread case class equality is used?