[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
https://github.com/dart-lang/sdk/issues/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 <johnniwinther@google.com>
Commit-Queue: Jens Johansen <jensj@google.com>
diff --git a/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart b/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
index c7c5dfc..9bad9b3 100644
--- a/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
+++ b/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
@@ -409,7 +409,7 @@
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 @@
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 @@
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 @@
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);
diff --git a/pkg/front_end/lib/src/fasta/source/source_loader.dart b/pkg/front_end/lib/src/fasta/source/source_loader.dart
index 08c9e9b..97db119 100644
--- a/pkg/front_end/lib/src/fasta/source/source_loader.dart
+++ b/pkg/front_end/lib/src/fasta/source/source_loader.dart
@@ -642,6 +642,9 @@
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 @@
}
/// 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) {
diff --git a/pkg/front_end/test/incremental_compiler_leak_tester.dart b/pkg/front_end/test/incremental_compiler_leak_test.dart
similarity index 80%
rename from pkg/front_end/test/incremental_compiler_leak_tester.dart
rename to pkg/front_end/test/incremental_compiler_leak_test.dart
index d05460b..581d63f 100644
--- a/pkg/front_end/test/incremental_compiler_leak_tester.dart
+++ b/pkg/front_end/test/incremental_compiler_leak_test.dart
@@ -85,30 +85,48 @@
// 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 @@
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,13 +272,13 @@
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;
@@ -262,4 +286,20 @@
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}";
+ }
}
