Skip to content

Unnecessary reference duplicate in backend #23681

@guillaumecarraux

Description

@guillaumecarraux

Compiler version

3.8.0-RC1 (nightly)

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 fix is also accessible in this fork.

n.b. in the case where this is accepted, a few tests would need to be changed (for ex. DottyBytecodeTests:patmatControlFlow does not match the bytecode anymore)

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions