[CFE] Fix incremental_compiler_leak_test(er)

In https://dart-review.googlesource.com/c/sdk/+/214020
incremental_compiler_leak_test was renamed to
incremental_compiler_leak_tester temporarily because it started saying
we had possible leaks.

Looking into it now, a number of puzzeling this were found:
* Many SourceLibraryBuilders, SourceClassBuilders etc was alive despite
  having been converted to DillLibraryBuilders (etc). They shoudn't be.
* Several Librarys (from kernels AST) came and went between
  invalidations.

This all was caused by the VM sometimes keeping things around longer
than it should (probably a variation of
#36983).

This CL fixes it by explicitley nulling things out (and similar).
It re-enasbles the test and updates it to explicitley expecting a
certain number of instances for some classes (e.g. it now expect
0 SourceLibraryBuilders).

To be clear I don't think this has caused any "real" leaks, but:
* having things clear up too late is very confusing.
* we will probably use less memory now.

Change-Id: I3a439f23fc7ef26b156c6164a2c37f6352bc57b4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/229964
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/front_end/lib/src/fasta/kernel/kernel_target.dart

@@ -409,7 +409,7 @@ class KernelTarget extends TargetImplementation {
       if (macroApplications != null) {
         await macroApplications.applyTypeMacros();
       }
-      List<SourceClassBuilder> sourceClassBuilders =
+      List<SourceClassBuilder>? sourceClassBuilders =
           loader.checkSemantics(objectClassBuilder);
       loader.finishTypeVariables(objectClassBuilder, dynamicType);
       loader.createTypeInferenceEngine();
@@ -442,6 +442,11 @@ class KernelTarget extends TargetImplementation {
       loader.checkMainMethods();
       installAllComponentProblems(loader.allComponentProblems);
       loader.allComponentProblems.clear();
+      // For whatever reason sourceClassBuilders is kept alive for some amount
+      // of time, meaning that all source library builders will be kept alive
+      // (for whatever amount of time) even though we convert them to dill
+      // library builders. To avoid it we null it out here.
+      sourceClassBuilders = null;
       return new BuildResult(
           component: component, macroApplications: macroApplications);
     }, () => loader.currentUriForCrashReporting);
@@ -467,7 +472,7 @@ class KernelTarget extends TargetImplementation {
       finishSynthesizedParameters();
       loader.finishDeferredLoadTearoffs();
       loader.finishNoSuchMethodForwarders();
-      List<SourceClassBuilder> sourceClasses = loader.collectSourceClasses();
+      List<SourceClassBuilder>? sourceClasses = loader.collectSourceClasses();
       if (macroApplications != null) {
         await macroApplications.applyDefinitionMacros(
             loader.coreTypes, loader.hierarchy);
@@ -479,6 +484,12 @@ class KernelTarget extends TargetImplementation {
 
       if (verify) this.verify();
       installAllComponentProblems(loader.allComponentProblems);
+
+      // For whatever reason sourceClasses is kept alive for some amount
+      // of time, meaning that all source library builders will be kept alive
+      // (for whatever amount of time) even though we convert them to dill
+      // library builders. To avoid it we null it out here.
+      sourceClasses = null;
       return new BuildResult(
           component: component, macroApplications: macroApplications);
     }, () => loader.currentUriForCrashReporting);

pkg/front_end/lib/src/fasta/source/source_loader.dart

@@ -642,6 +642,9 @@ class SourceLoader extends Loader {
       currentUriForCrashReporting = library.importUri;
       await buildBody(library);
     }
+    // Workaround: This will return right away but avoid a "semi leak"
+    // where the latest library is saved in a context somewhere.
+    await buildBody(null);
     currentUriForCrashReporting = null;
     logSummary(templateSourceBodySummary);
   }
@@ -1076,7 +1079,10 @@ severity: $severity
   }
 
   /// Builds all the method bodies found in the given [library].
-  Future<Null> buildBody(SourceLibraryBuilder library) async {
+  Future<Null> buildBody(SourceLibraryBuilder? library) async {
+    // [library] is only nullable so we can call this a "dummy-time" to get rid
+    // of a semi-leak.
+    if (library == null) return;
     Iterable<SourceLibraryBuilder>? patches = library.patchLibraries;
     if (patches != null) {
       for (SourceLibraryBuilder patchLibrary in patches) {

pkg/front_end/test/incremental_compiler_leak_tester.dart renamed to pkg/front_end/test/incremental_compiler_leak_test.dart

@@ -85,30 +85,48 @@ class LeakFinder extends vmService.LaunchingVMServiceHelper {
       // For now ignore anything not in package:kernel or package:front_end.
       if (ignoredClass(classDetails)) continue;
 
+      bool isStrictClass = strictClass(classDetails);
+
+      int expectedStrictClassNumber = -1;
+      if (isStrictClass) {
+        expectedStrictClassNumber = strictClassExpectedNumber(classDetails);
+      }
+
       // If they're all equal there's nothing to talk about.
-      bool same = true;
-      for (int i = 1; i < listOfInstanceCounts.length; i++) {
+      bool sameAndAsExpected = true;
+      for (int i = 0; i < listOfInstanceCounts.length; i++) {
+        if (expectedStrictClassNumber > -1 &&
+            expectedStrictClassNumber != listOfInstanceCounts[i]) {
+          sameAndAsExpected = false;
+          break;
+        }
         if (listOfInstanceCounts[i] != listOfInstanceCounts[0]) {
-          same = false;
+          sameAndAsExpected = false;
           break;
         }
       }
-      if (same) continue;
+      if (sameAndAsExpected) continue;
 
       int midPoint = listOfInstanceCounts.length ~/ 2;
       List<int> firstHalf = listOfInstanceCounts.sublist(0, midPoint);
       List<int> secondHalf = listOfInstanceCounts.sublist(midPoint);
       TTestResult ttestResult = SimpleTTestStat.ttest(secondHalf, firstHalf);
 
-      if (!strictClass(classDetails)) {
+      if (!isStrictClass) {
         if (!ttestResult.significant) continue;
 
         // TODO(jensj): We could possibly also ignore if it's less (i.e. a
         // negative change), or if the change is < 1%, or the change minus the
         // confidence is < 1% etc.
       }
-      print("Differences on ${c.name} (${uriString}): "
-          "$listOfInstanceCounts ($ttestResult)");
+      if (expectedStrictClassNumber > -1) {
+        print("Differences on ${c.name} (${uriString}): "
+            "Expected exactly $expectedStrictClassNumber but found "
+            "$listOfInstanceCounts ($ttestResult)");
+      } else {
+        print("Differences on ${c.name} (${uriString}): "
+            "$listOfInstanceCounts ($ttestResult)");
+      }
       foundLeak = true;
     }
 
@@ -219,20 +237,26 @@ class LeakFinder extends vmService.LaunchingVMServiceHelper {
     return true;
   }
 
-  // I have commented out the lazy ones below.
-  Set<String> frontEndStrictClasses = {
+  Map<String, int> frontEndStrictClasses = {
+    // The inner working of dills are created lazily:
     // "DillClassBuilder",
     // "DillExtensionBuilder",
     // "DillExtensionMemberBuilder",
-    "DillLibraryBuilder",
-    "DillLoader",
     // "DillMemberBuilder",
-    "DillTarget",
     // "DillTypeAliasBuilder",
-    "SourceClassBuilder",
-    "SourceExtensionBuilder",
-    "SourceLibraryBuilder",
-    "SourceLoader",
+
+    "DillLibraryBuilder": -1 /* unknown amount */,
+    "DillLoader": 1,
+    "DillTarget": 1,
+
+    // We convert all source builders to dill builders so we expect none to
+    // exist after that.
+    "SourceClassBuilder": 0,
+    "SourceExtensionBuilder": 0,
+    "SourceLibraryBuilder": 0,
+
+    // We still expect exactly 1 source loader though.
+    "SourceLoader": 1,
   };
 
   Set<String> kernelAstStrictClasses = {
@@ -248,18 +272,34 @@ class LeakFinder extends vmService.LaunchingVMServiceHelper {
 
   bool strictClass(vmService.Class classDetails) {
     if (!kernelAstStrictClasses.contains(classDetails.name) &&
-        !frontEndStrictClasses.contains(classDetails.name)) return false;
+        !frontEndStrictClasses.containsKey(classDetails.name)) return false;
 
     if (kernelAstStrictClasses.contains(classDetails.name) &&
         classDetails.location?.script?.uri == "package:kernel/ast.dart") {
       return true;
     }
-    if (frontEndStrictClasses.contains(classDetails.name) &&
+    if (frontEndStrictClasses.containsKey(classDetails.name) &&
         classDetails.location?.script?.uri?.startsWith("package:front_end/") ==
             true) {
       return true;
     }
 
     throw "$classDetails: ${classDetails.name} --- ${classDetails.location}";
   }
+
+  int strictClassExpectedNumber(vmService.Class classDetails) {
+    if (!strictClass(classDetails)) return -1;
+    if (kernelAstStrictClasses.contains(classDetails.name) &&
+        classDetails.location?.script?.uri == "package:kernel/ast.dart") {
+      return -1;
+    }
+    int? result = frontEndStrictClasses[classDetails.name];
+    if (result != null &&
+        classDetails.location?.script?.uri?.startsWith("package:front_end/") ==
+            true) {
+      return result;
+    }
+
+    throw "$classDetails: ${classDetails.name} --- ${classDetails.location}";
+  }
 }