Skip to content

Commit 28f95fc

Browse files
jensjohacommit-bot@chromium.org
jensjoha
authored and
[email protected]
committed
[cfe] Track dependencies in the incremental compiler
The objective of this CL is to allow the incremental compiler to track which dill input libraries it actually used (in order to - in a modular context - be able to ignore changes to dependencies we didn't really use). This is done by: * Making the dill library builder lazy (and mark if it has been used). * Making kernels class hierarchy record which classes it has been asked questions about. * Add special handling to redirecting factory constructors as they bypass the fasta-builders and directly use the kernel ast. Please note that: * This has to be enabled, but kernels class hierarchy always records which classes it was asked about (even if disabled, or not running though the incremental compiler). There might potentially be some overhead to this (though setting a bool to true is probably comparably cheap - we did just do a map lookup). * This was designed to be used together with modules (e.g. setModulesToLoadOnNextComputeDelta) - as used via for instance api_unstable/bazel_worker.dart. It might not function the way you'd expect in other circumstances. * The incremental compiler (potentially) gives you the full kernel tree despite not having marked all of those libraries as 'used'. If a client use such libraries it is the clients responsibility to take that into account when answering any questions about this libraries/dill files was used. * This feature works on the library level. In practice it is most likely needed at the dill-filename level. A translation from library to dill-filename is up to the client. * This is a new feature, and we cannot promise that 100% of actually used libraries are marked. If you find used but un-marked libraries please report a bug. Change-Id: I01d7ff95b9baac9550b77d8e09ea772d43173641 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/107280 Commit-Queue: Jens Johansen <[email protected]> Reviewed-by: Johnni Winther <[email protected]>
1 parent afac6b1 commit 28f95fc

17 files changed

+830
-76
lines changed

‎pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart‎

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,31 @@ import 'dill_loader.dart' show DillLoader;
5151

5252
import 'dill_type_alias_builder.dart' show DillTypeAliasBuilder;
5353

54+
class LazyLibraryScope extends Scope {
55+
DillLibraryBuilder libraryBuilder;
56+
57+
LazyLibraryScope(Map<String, Declaration> local,
58+
Map<String, Declaration> setters, Scope parent, String debugName,
59+
{bool isModifiable: true})
60+
: super(local, setters, parent, debugName, isModifiable: isModifiable);
61+
62+
LazyLibraryScope.top({bool isModifiable: false})
63+
: this(<String, Declaration>{}, <String, Declaration>{}, null, "top",
64+
isModifiable: isModifiable);
65+
66+
Map<String, Declaration> get local {
67+
if (libraryBuilder == null) throw new StateError("No library builder.");
68+
libraryBuilder.ensureLoaded();
69+
return super.local;
70+
}
71+
72+
Map<String, Declaration> get setters {
73+
if (libraryBuilder == null) throw new StateError("No library builder.");
74+
libraryBuilder.ensureLoaded();
75+
return super.setters;
76+
}
77+
}
78+
5479
class DillLibraryBuilder extends LibraryBuilder<KernelTypeBuilder, Library> {
5580
final Library library;
5681

@@ -64,8 +89,38 @@ class DillLibraryBuilder extends LibraryBuilder<KernelTypeBuilder, Library> {
6489

6590
bool exportsAlreadyFinalized = false;
6691

92+
// TODO(jensj): These 4 booleans could potentially be merged into a single
93+
// state field.
94+
bool isReadyToBuild = false;
95+
bool isReadyToFinalizeExports = false;
96+
bool isBuilt = false;
97+
bool isBuiltAndMarked = false;
98+
6799
DillLibraryBuilder(this.library, this.loader)
68-
: super(library.fileUri, new Scope.top(), new Scope.top());
100+
: super(library.fileUri, new LazyLibraryScope.top(),
101+
new LazyLibraryScope.top()) {
102+
LazyLibraryScope lazyScope = scope;
103+
lazyScope.libraryBuilder = this;
104+
LazyLibraryScope lazyExportScope = exportScope;
105+
lazyExportScope.libraryBuilder = this;
106+
}
107+
108+
void ensureLoaded() {
109+
if (!isReadyToBuild) throw new StateError("Not ready to build.");
110+
isBuiltAndMarked = true;
111+
if (isBuilt) return;
112+
isBuilt = true;
113+
library.classes.forEach(addClass);
114+
library.procedures.forEach(addMember);
115+
library.typedefs.forEach(addTypedef);
116+
library.fields.forEach(addMember);
117+
118+
if (isReadyToFinalizeExports) {
119+
finalizeExports();
120+
} else {
121+
throw new StateError("Not ready to finalize exports.");
122+
}
123+
}
69124

70125
@override
71126
bool get isSynthetic => library.isSynthetic;
@@ -174,6 +229,14 @@ class DillLibraryBuilder extends LibraryBuilder<KernelTypeBuilder, Library> {
174229
return library.name ?? "<library '${library.fileUri}'>";
175230
}
176231

232+
void markAsReadyToBuild() {
233+
isReadyToBuild = true;
234+
}
235+
236+
void markAsReadyToFinalizeExports() {
237+
isReadyToFinalizeExports = true;
238+
}
239+
177240
void finalizeExports() {
178241
if (exportsAlreadyFinalized) return;
179242
exportsAlreadyFinalized = true;

‎pkg/front_end/lib/src/fasta/dill/dill_loader.dart‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,7 @@ class DillLoader extends Loader<Library> {
6161
if (builder.library == null) {
6262
unhandled("null", "builder.library", 0, builder.fileUri);
6363
}
64-
builder.library.classes.forEach(builder.addClass);
65-
builder.library.procedures.forEach(builder.addMember);
66-
builder.library.typedefs.forEach(builder.addTypedef);
67-
builder.library.fields.forEach(builder.addMember);
64+
builder.markAsReadyToBuild();
6865
}
6966

7067
Future<Null> buildBody(DillLibraryBuilder builder) {
@@ -74,7 +71,7 @@ class DillLoader extends Loader<Library> {
7471
void finalizeExports() {
7572
builders.forEach((Uri uri, LibraryBuilder builder) {
7673
DillLibraryBuilder library = builder;
77-
library.finalizeExports();
74+
library.markAsReadyToFinalizeExports();
7875
});
7976
}
8077

‎pkg/front_end/lib/src/fasta/incremental_compiler.dart‎

Lines changed: 105 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ import 'package:kernel/binary/ast_from_binary.dart'
1515
CanonicalNameSdkError,
1616
InvalidKernelVersionError;
1717

18-
import 'package:kernel/class_hierarchy.dart' show ClassHierarchy;
18+
import 'package:kernel/class_hierarchy.dart'
19+
show ClassHierarchy, ClosedWorldClassHierarchy;
1920

2021
import 'package:kernel/kernel.dart'
2122
show
@@ -32,6 +33,7 @@ import 'package:kernel/kernel.dart'
3233
ProcedureKind,
3334
ReturnStatement,
3435
Source,
36+
Supertype,
3537
TreeNode,
3638
TypeParameter;
3739

@@ -68,6 +70,8 @@ import 'fasta_codes.dart'
6870

6971
import 'hybrid_file_system.dart' show HybridFileSystem;
7072

73+
import 'kernel/kernel_builder.dart' show ClassHierarchyBuilder;
74+
7175
import 'kernel/kernel_library_builder.dart' show KernelLibraryBuilder;
7276

7377
import 'kernel/kernel_shadow_ast.dart' show VariableDeclarationJudgment;
@@ -90,6 +94,8 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
9094
final Ticker ticker;
9195

9296
final bool outlineOnly;
97+
bool trackNeededDillLibraries = false;
98+
Set<Library> neededDillLibraries;
9399

94100
Set<Uri> invalidatedUris = new Set<Uri>();
95101

@@ -301,6 +307,19 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
301307
uriTranslator);
302308
userCode.loader.hierarchy = hierarchy;
303309

310+
if (trackNeededDillLibraries) {
311+
// Reset dill loaders and kernel class hierarchy.
312+
for (LibraryBuilder builder in dillLoadedData.loader.builders.values) {
313+
if (builder is DillLibraryBuilder) {
314+
builder.isBuiltAndMarked = false;
315+
}
316+
}
317+
318+
if (hierarchy is ClosedWorldClassHierarchy) {
319+
hierarchy.resetUsed();
320+
}
321+
}
322+
304323
for (LibraryBuilder library in reusedLibraries) {
305324
userCode.loader.builders[library.uri] = library;
306325
if (library.uri.scheme == "dart" && library.uri.path == "core") {
@@ -324,8 +343,23 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
324343
componentWithDill =
325344
await userCode.buildComponent(verify: c.options.verify);
326345
}
346+
hierarchy ??= userCode.loader.hierarchy;
327347

328348
recordNonFullComponentForTesting(componentWithDill);
349+
if (trackNeededDillLibraries) {
350+
// Which dill builders were built?
351+
neededDillLibraries = new Set<Library>();
352+
for (LibraryBuilder builder in dillLoadedData.loader.builders.values) {
353+
if (builder is DillLibraryBuilder) {
354+
if (builder.isBuiltAndMarked) {
355+
neededDillLibraries.add(builder.library);
356+
}
357+
}
358+
}
359+
360+
updateNeededDillLibraresWithHierarchy(
361+
hierarchy, userCode.loader.builderHierarchy);
362+
}
329363

330364
if (componentWithDill != null) {
331365
this.invalidatedUris.clear();
@@ -388,6 +422,76 @@ class IncrementalCompiler implements IncrementalKernelGenerator {
388422
});
389423
}
390424

425+
/// Allows for updating the list of needed libraries.
426+
///
427+
/// Useful if a class hierarchy has been used externally.
428+
/// Currently there are two different class hierarchies which is unfortunate.
429+
/// For now this method allows the 'ClassHierarchyBuilder' to be null.
430+
///
431+
/// TODO(jensj,CFE in general): Eventually we should get to a point where we
432+
/// only have one class hierarchy.
433+
/// TODO(jensj): This could probably be a utility method somewhere instead
434+
/// (though handling of the case where all bets are off should probably still
435+
/// live locally).
436+
void updateNeededDillLibraresWithHierarchy(
437+
ClassHierarchy hierarchy, ClassHierarchyBuilder builderHierarchy) {
438+
if (hierarchy is ClosedWorldClassHierarchy && !hierarchy.allBetsOff) {
439+
neededDillLibraries ??= new Set<Library>();
440+
Set<Class> classes = new Set<Class>();
441+
List<Class> worklist = new List<Class>();
442+
// Get all classes touched by kernel class hierarchy.
443+
List<Class> usedClasses = hierarchy.getUsedClasses();
444+
worklist.addAll(usedClasses);
445+
classes.addAll(usedClasses);
446+
447+
// Get all classes touched by fasta class hierarchy.
448+
if (builderHierarchy != null) {
449+
for (Class c in builderHierarchy.nodes.keys) {
450+
if (classes.add(c)) worklist.add(c);
451+
}
452+
}
453+
454+
// Get all supers etc.
455+
while (worklist.isNotEmpty) {
456+
Class c = worklist.removeLast();
457+
for (Supertype supertype in c.implementedTypes) {
458+
if (classes.add(supertype.classNode)) {
459+
worklist.add(supertype.classNode);
460+
}
461+
}
462+
if (c.mixedInType != null) {
463+
if (classes.add(c.mixedInType.classNode)) {
464+
worklist.add(c.mixedInType.classNode);
465+
}
466+
}
467+
if (c.supertype != null) {
468+
if (classes.add(c.supertype.classNode)) {
469+
worklist.add(c.supertype.classNode);
470+
}
471+
}
472+
}
473+
474+
// Add any libraries that was used or was in the "parent-chain" of a
475+
// used class.
476+
for (Class c in classes) {
477+
Library library = c.enclosingLibrary;
478+
// Only add if loaded from a dill file.
479+
if (dillLoadedData.loader.builders.containsKey(library.importUri)) {
480+
neededDillLibraries.add(library);
481+
}
482+
}
483+
} else {
484+
// Cannot track in other kernel class hierarchies or
485+
// if all bets are off: Add everything.
486+
neededDillLibraries = new Set<Library>();
487+
for (LibraryBuilder builder in dillLoadedData.loader.builders.values) {
488+
if (builder is DillLibraryBuilder) {
489+
neededDillLibraries.add(builder.library);
490+
}
491+
}
492+
}
493+
}
494+
391495
/// Internal method.
392496
void invalidateNotKeptUserBuilders(Set<Uri> invalidatedUris) {
393497
if (modulesToLoad != null && userBuilders != null) {

‎pkg/front_end/lib/src/fasta/kernel/body_builder.dart‎

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import 'dart:core' hide MapEntry;
88

99
import '../constant_context.dart' show ConstantContext;
1010

11+
import '../dill/dill_library_builder.dart' show DillLibraryBuilder;
12+
1113
import '../fasta_codes.dart' as fasta;
1214

1315
import '../fasta_codes.dart' show LocatedMessage, Message, noLength, Template;
@@ -133,7 +135,7 @@ const noLocation = null;
133135
const invalidCollectionElement = const Object();
134136

135137
abstract class BodyBuilder extends ScopeListener<JumpTarget>
136-
implements ExpressionGeneratorHelper {
138+
implements ExpressionGeneratorHelper, EnsureLoaded {
137139
// TODO(ahe): Rename [library] to 'part'.
138140
@override
139141
final KernelLibraryBuilder library;
@@ -903,6 +905,44 @@ abstract class BodyBuilder extends ScopeListener<JumpTarget>
903905
finishVariableMetadata();
904906
}
905907

908+
/// Ensure that the containing library of the [member] has been loaded.
909+
///
910+
/// This is for instance important for lazy dill library builders where this
911+
/// method has to be called to ensure that
912+
/// a) The library has been fully loaded (and for instance any internal
913+
/// transformation needed has been performed); and
914+
/// b) The library is correctly marked as being used to allow for proper
915+
/// 'dependency pruning'.
916+
void ensureLoaded(Member member) {
917+
if (member == null) return;
918+
Library ensureLibraryLoaded = member.enclosingLibrary;
919+
LibraryBuilder<dynamic, dynamic> builder =
920+
library.loader.builders[ensureLibraryLoaded.importUri] ??
921+
library.loader.target.dillTarget.loader
922+
.builders[ensureLibraryLoaded.importUri];
923+
if (builder is DillLibraryBuilder) {
924+
builder.ensureLoaded();
925+
}
926+
}
927+
928+
/// Check if the containing library of the [member] has been loaded.
929+
///
930+
/// This is designed for use with asserts.
931+
/// See [ensureLoaded] for a description of what 'loaded' means and the ideas
932+
/// behind that.
933+
bool isLoaded(Member member) {
934+
if (member == null) return true;
935+
Library ensureLibraryLoaded = member.enclosingLibrary;
936+
LibraryBuilder<dynamic, dynamic> builder =
937+
library.loader.builders[ensureLibraryLoaded.importUri] ??
938+
library.loader.target.dillTarget.loader
939+
.builders[ensureLibraryLoaded.importUri];
940+
if (builder is DillLibraryBuilder) {
941+
return builder.isBuiltAndMarked;
942+
}
943+
return true;
944+
}
945+
906946
void resolveRedirectingFactoryTargets() {
907947
for (StaticInvocation invocation in redirectingFactoryInvocations) {
908948
// If the invocation was invalid, it or its parent has already been
@@ -927,7 +967,7 @@ abstract class BodyBuilder extends ScopeListener<JumpTarget>
927967
Expression replacementNode;
928968

929969
RedirectionTarget redirectionTarget =
930-
getRedirectionTarget(initialTarget, legacyMode: legacyMode);
970+
getRedirectionTarget(initialTarget, this, legacyMode: legacyMode);
931971
Member resolvedTarget = redirectionTarget?.target;
932972

933973
if (resolvedTarget == null) {
@@ -3355,7 +3395,7 @@ abstract class BodyBuilder extends ScopeListener<JumpTarget>
33553395
int charLength: noLength}) {
33563396
// The argument checks for the initial target of redirecting factories
33573397
// invocations are skipped in Dart 1.
3358-
if (!legacyMode || !isRedirectingFactory(target)) {
3398+
if (!legacyMode || !isRedirectingFactory(target, helper: this)) {
33593399
List<TypeParameter> typeParameters = target.function.typeParameters;
33603400
if (target is Constructor) {
33613401
assert(!target.enclosingClass.isAbstract);
@@ -3664,7 +3704,7 @@ abstract class BodyBuilder extends ScopeListener<JumpTarget>
36643704
(target is Procedure && target.kind == ProcedureKind.Factory)) {
36653705
Expression invocation;
36663706

3667-
if (legacyMode && isRedirectingFactory(target)) {
3707+
if (legacyMode && isRedirectingFactory(target, helper: this)) {
36683708
// In legacy mode the checks that are done in [buildStaticInvocation]
36693709
// on the initial target of a redirecting factory invocation should
36703710
// be skipped. So we build the invocation nodes directly here without
@@ -3690,7 +3730,8 @@ abstract class BodyBuilder extends ScopeListener<JumpTarget>
36903730
charLength: nameToken.length);
36913731
}
36923732

3693-
if (invocation is StaticInvocation && isRedirectingFactory(target)) {
3733+
if (invocation is StaticInvocation &&
3734+
isRedirectingFactory(target, helper: this)) {
36943735
redirectingFactoryInvocations.add(invocation);
36953736
}
36963737

@@ -5408,6 +5449,11 @@ abstract class BodyBuilder extends ScopeListener<JumpTarget>
54085449
}
54095450
}
54105451

5452+
abstract class EnsureLoaded {
5453+
void ensureLoaded(Member member);
5454+
bool isLoaded(Member member);
5455+
}
5456+
54115457
class Operator {
54125458
final Token token;
54135459
String get name => token.stringValue;

0 commit comments

Comments
 (0)