native icon indicating copy to clipboard operation
native copied to clipboard

Test that NativeFinalizer is actually invoked.

Open mahesh-hegde opened this issue 2 years ago • 4 comments
trafficstars

This was originally a checkbox under dart-lang/native#669 . But I think this deserves separate issue.

Have to verify the NativeFinalizer is actually invoked when GC is invoked.

We should either

  • Mock the finalizer in this test
    • How?
  • Associate an atomic counter or keep some other metric in dartjni.so

https://github.com/dart-lang/ffigen/blob/master/test/native_objc_test/automated_ref_count_test.dart is the test Liam mentioned in a previous discussion but I don't think this counter-decrease-on-delete pattern is applicable in Java.

  • On Android, Dart_ExecuteInternalCommand symbol does not exist
    • Is there another way to trigger GC on Android?

cc: @liamappelbe @dcharkes

mahesh-hegde avatar Apr 22 '23 09:04 mahesh-hegde

The only way that I know of without Dart_ExecuteInternalCommand is to spawn a new isolate group, attach a NativeFinalizer to a Finalizable there, and shutdown that isolate group. And then to to check that the finalizer ran, increment some integer in a memory location by one when running the native finalizer.

Though, you shouldn't have to test that native finalizers work. We have tests in the Dart SDK for that.

Have to verify the NativeFinalizer is actually invoked when GC is invoked.

Note that Dart is a generational garbage collector with an old a and new space.

https://medium.com/flutter/flutter-dont-fear-the-garbage-collector-d69b3ff1ca30

So there are both old and new space garbage collection cycles, and if an object lives in the other space than the one being collected, it will not be collected. (Also there are some cases in which the object in old space is referred to from new space garbage, in that case even an old space collection doesn't collect the object, it will only do so after the new-space garbage has been collected or promoted to old space itself.) TL;DR: at some point garbage will be collected, but no hard guarantees.

@mahesh-hegde what would you actually want to test? Do we want to test that we clean up garbage in general? E.g. that we are not forgetting to clean up.

Maybe we should have an atomic counter around creating and deleting global references, and see that that number goes down again in a longer running test?

dcharkes avatar Apr 24 '23 08:04 dcharkes

what would you actually want to test? Do we want to test that we clean up garbage in general? E.g. that we are not forgetting to clean up.

Yes.

Maybe we should have an atomic counter around creating and deleting global references, and see that that number goes down again in a longer running test?

Is this reliable in a test environment? There will be other tests running, we wouldn't have much time to do the test, and this would ultimately depend on GC implementation details?

mahesh-hegde avatar Apr 24 '23 13:04 mahesh-hegde

I was thinking the atomic counter could also be useful for ensuring that we don't hit the 51200 global refs.

I'd probably not make it part of the normal test suite, but some separate longer running script.

Yeah, it would depend on when the GC decides to run.

Maybe a better test is to make a longer running script that will most definitely fail if the native finalizers are not run. For example allocating global refs in a loop and forgetting about them. And then doing that more than 51200 times. 🙊 But trying to write the script in such a way that you also generate enough Dart objects that need to be collected.

Testing these things are hard, we know that finalizers themselves work, that's been thoroughly covered in the SDK test suites. So maybe we need to just rely on code-review that we attach finalizers to global refs.

dcharkes avatar Apr 24 '23 14:04 dcharkes

It's a good idea to add a metric interface, and long running separate test.

From an organization perspective, let's detach this from dart-lang/native#669 for now. In dart-lang/native#669 I will assume NativeFinalizer will run and verify more boring cases.

mahesh-hegde avatar Apr 24 '23 14:04 mahesh-hegde