Local is not cleared, causing an object not to be GC'd

[durban]

Compiler version

3.1.0

Minimized code

import java.lang.ref.WeakReference
import java.util.concurrent.atomic.AtomicReference

final class Mark

final object MyTest {

  def main(args: Array[String]): Unit = {
    myTest()
  }

  def myAssert(cond: => Boolean): Unit = {
    assert(cond)
  }

  def myTest(): Unit = {
    val ref = new AtomicReference[WeakReference[AnyRef]]
    var mark: AnyRef = null
    assert(ref.compareAndSet(null, new WeakReference(new Mark)))
    mark = ref.get().get()
    myAssert(mark ne null) // in theory this could fail, but it isn't
    mark = null
    while (ref.get().get() ne null) {
      System.gc()
      Thread.sleep(1L)
    }
  }
}

Output

The program never exits.

Expectation

I think eventually the GC should collect the Mark instance, and the while loop should finish. (Works fine on Scala 2.13.7.)

Looking at a heap dump, the Mark instance is a GC root.

Looking at the JVM bytecode, it seems there is an "unnamed" local variable in the method myTest (i.e., it is not in the LocalVariableTable). The LocalVariableTable contains slots 0 (this), 1 (ref), and 2 (mark). In the bytecode there is a store to slot 3:

        49: invokevirtual #78                 // Method java/lang/ref/Reference.get:()Ljava/lang/Object;
        52: astore_3
        53: aload_2
        54: aload_3
        55: putfield      #82                 // Field scala/runtime/ObjectRef.elem:Ljava/lang/Object;

I think 49: invokevirtual is the second .get() in the line mark = ref.get().get(). Then 52: astore_3 is the store to this "unnamed local 3". Then 55: putfield stores it in the ObjectRef corresponding to the var mark.

However, when later we (try to) clear the reference in the line mark = null, only the ObjectRef is cleared, the "unnamed local 3" isn't:

        76: aconst_null
        77: putfield      #82                 // Field scala/runtime/ObjectRef.elem:Ljava/lang/Object;

Interestingly, right before this, there is a store of null to another unnamed local, in slot 4 (I couldn't figure out, what is this local 4):

        69: aconst_null
        70: astore        4
        72: aload_2
        73: aload         4
        75: pop

Another observation: the myAssert seems necessary to reproduce the issue (with a normal assert, it works fine).

[smarter]

@lrytz does this ring any bell / any idea what part of the backend could be responsible for this?

[som-snytt]

I guess this is the question from the other day. By coincidence, I revived the lint for boxing of captured vars. It triggers in lambdalift but doesn't yet account for optimization in the backend. If Lukas drops some hints, I will pursue them while digging in the area.

[lrytz]

Looking at -Xprint:all, the local variable seems to be added here:

[[syntax trees at end of                   erasure]] // Test.scala

        mark = ref.get().asInstanceOf[java.lang.ref.Reference].get()


[[syntax trees at end of MegaPhase{elimErasedValueType, pureStats, vcElideAllocations, arrayApply, elimPolyFunction, tailrec, completeJavaEnums, mixin, lazyVals, memoize, nonLocalReturns, capturedVars}]] // Test.scala


        {
          val ev$1: Object =
            ref.get().asInstanceOf[java.lang.ref.Reference].get()
          mark.elem = ev$1
        }

[som-snytt]

Why it's done that way https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/CapturedVars.scala#L134

If it's just a matter of clearing the temporary, I'll try to PR it. Or maybe it's also worth avoiding the temporary when feasible.

Things you don't think about when assigning to a variable.

[som-snytt]

Thanks lrytz, I don't know why they always blame the backend. :) This was a good first issue. Or it will have been a good first issue, if I can get remaining tests to pass.

[smarter]

I had never considered that local variables in the AST could lead to such issues, there's many situations where we create temporary variables in local blocks like this (grep for evalOnce / letBindUnless), I have no idea if they could lead to the same issue though.

[som-snytt]

I did look at uses of SyntheticValDef because of the lack of a SyntheticVarDef. (I learned that you can assign to a val unless they said -Ycheck:all.) By good first issue, I mean I know how to assign to a variable, but trying to do it in this context requires learning a lot of context.