[vm] Optimizer can keep object longer alive due to CSE/Store-to-Load of WeakReference.target

[mkustermann]

The following example (from cl/363581):

Future testWeakReferenceWeakness() async {
  late final WeakReference<Foo> weakReference;
  {
    Foo? foo = Foo();
    weakReference = WeakReference<Foo>(foo);
    Expect.equals(weakReference.target, foo);
    foo = null;
  }
  asyncStart();
  while (weakReference.target != null) {
    produceGarbage();
    await Future.delayed(const Duration(milliseconds: 10));
  }
  Expect.isNull(weakReference.target);
  asyncEnd();
}

is allocating an object that becomes immediately unreachable. We then produce garbage until the object was collected and the weak reference to it null'ed out.

This works in JIT mode but in AOT it seems that the optimizer is going to optimize it (incorrectly IMHO) away by forwarding the allocated object to the load from the WeakReference.

This is proof that the optimizer can prevent collection of unreachable objects.

/cc @alexmarkov @mraleph opinions on this behavior?

[mkustermann]

Approved the tests

corelib/weak_reference_test Timeout (expected Pass)

are failing on configurations

vm-aot-linux-release-simarm_x64
vm-aot-linux-release-x64
vm-aot-win-release-x64

[alexmarkov]

We can disable load forwarding specifically for WeakReference.target field, as it doesn't behave like other fields: subsequent load after previous load or store is not guaranteed to return the same result. This can be done in IsLoadEliminationCandidate:

diff --git a/runtime/vm/compiler/backend/redundancy_elimination.cc b/runtime/vm/compiler/backend/redundancy_elimination.cc
index 851635c713d..f71c8585af0 100644
--- a/runtime/vm/compiler/backend/redundancy_elimination.cc
+++ b/runtime/vm/compiler/backend/redundancy_elimination.cc
@@ -1427,6 +1427,11 @@ static bool IsLoadEliminationCandidate(Instruction* instr) {
       instr->AsDefinition()->MayCreateUnsafeUntaggedPointer()) {
     return false;
   }
+  if (auto* load = instr->AsLoadField()) {
+    if (load->slot().kind() == Slot::Kind::kWeakReference_target) {
+      return false;
+    }
+  }
   return instr->IsLoadField() || instr->IsLoadIndexed() ||
          instr->IsLoadStaticField();
 }

On the other hand, WeakReferences are not guaranteed to be cleared, so this particular test is probably incorrect and optimization is valid.

[alexmarkov]

The test corelib/weak_reference_test causes canceled builds on certain bots due to very long deflaking (https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8750074602117134321/+/u/deflaking/vm_tests/stdout).

https://dart-review.googlesource.com/c/sdk/+/363960 attempts to fix the test to conform to the WeakReference specification:

sdk/sdk/lib/core/weak.dart

Lines 75 to 76 in a9f684e

	/// There are no guarantees that a weak reference will ever be cleared
	/// even if all references to its target are weak references.

(We should test according to the specification, not implementation.)

[rmacnak-google]

We should disable forwarding for all the non-strong fields: WeakReference.target, WeakArray.<element>, WeakProperty.key/value, FinalizerEntry.value/detach/finalizer. These fields are all subject to change at any safepoint.

[mraleph]

I agree with @rmacnak-google - we should disable the forwarding. It will yield more understandable behavior and access to these fields should not be performance sensitive.

[alexmarkov]

I agree that it would be better to disable load forwarding of weak fields. As there are no objections, I'll create a CL for that.

However, I'm a bit skeptical that fixing this particular case would allow us to provide more strict guarantees around clearing of weak references: load forwarding is not the only way the life time of objects is extended beyond their scope.

A couple of examples:

Extending object life time through a local variable slot in unoptimized JIT (foo goes out of scope):

Future testWeakReferenceWeakness() async {
  late final WeakReference<Foo> weakReference;
  {
    Foo? foo = Foo();
    weakReference = WeakReference<Foo>(foo);
    Expect.equals(weakReference.target, foo);
  }
  asyncStart();
  while (weakReference.target != null) {
    produceGarbage();
    await Future.delayed(const Duration(milliseconds: 10));
  }
  Expect.isNull(weakReference.target);
  asyncEnd();
}

Extending life time through the closure context (all modes):

Future testWeakReferenceWeakness() async {
  late final WeakReference<Foo> weakReference;
  {
    Foo? foo = Foo();
    weakReference = WeakReference<Foo>(foo);
    Expect.equals(weakReference.target, foo);
    print(weakReference); // Leak object to disable load forwarding.
    // Weak reference is cleared (in AOT/opt JIT) if this closure is removed.
    Function? f1 = () {
      print(foo);
      print(weakReference);
    };
    print(f1);
    f1 = null;
  }
  asyncStart();
  while (weakReference.target != null) {
    produceGarbage();
    await Future.delayed(const Duration(milliseconds: 10));
  }
  Expect.isNull(weakReference.target);
  asyncEnd();
}

This list is not exhaustive, there are probably other cases of extending life time of objects.