Change in dart/sdk[master]: [bazel] Add retry to kernel_worker on 'wrong' input
1 view
Skip to first unread message
Jens Johansen (Gerrit)
Aug 14, 2019, 4:06:13 AM8/14/19
to dart-fe-te...@google.com, rev...@dartlang.org, Jake Macdonald
I'm not sure whether this should land or not. What do you think?
Patch set 2:Commit-Queue +1
To view, visit change 113023. To unsubscribe, or for help writing mail filters, visit settings.
Jens Johansen (Gerrit)
Aug 14, 2019, 4:06:14 AM8/14/19
to Jake Macdonald, dart-fe-te...@google.com, rev...@dartlang.org
Jens Johansen would like Jake Macdonald to review this change.
[bazel] Add retry to kernel_worker on 'wrong' input
For instance, loading the same file from different paths is going to
cause trouble.
This CL detects if we have the same library from different input dills,
and throws away the cache if it does.
If we still have the same situation after that, it means that the files
we were explicitly told to load now contains the same library more than
once and it throws a state error.
Change-Id: I27f07e756b5d086edde308a8d3a1b1c0fcf8a7bf
---
M pkg/front_end/lib/src/api_unstable/bazel_worker.dart
M pkg/front_end/lib/src/api_unstable/compiler_state.dart
2 files changed, 150 insertions(+), 95 deletions(-)
diff --git a/pkg/front_end/lib/src/api_unstable/bazel_worker.dart b/pkg/front_end/lib/src/api_unstable/bazel_worker.dart
index de1a13c..fb8e89b 100644
--- a/pkg/front_end/lib/src/api_unstable/bazel_worker.dart
+++ b/pkg/front_end/lib/src/api_unstable/bazel_worker.dart
@@ -8,7 +8,7 @@
import 'dart:async' show Future;
import 'package:front_end/src/api_prototype/compiler_options.dart';
-import 'package:kernel/kernel.dart' show Component, CanonicalName;
+import 'package:kernel/kernel.dart' show Component, CanonicalName, Library;
import 'package:kernel/target/targets.dart' show Target;
@@ -75,6 +75,9 @@
WorkerInputComponent cachedSdkInput;
Map<Uri, WorkerInputComponent> workerInputCache =
oldState?.workerInputCache ?? new Map<Uri, WorkerInputComponent>();
+ Map<Uri, Uri> workerInputCacheLibs =
+ oldState?.workerInputCacheLibs ?? new Map<Uri, Uri>();
+
bool startOver = false;
Map<ExperimentalFlag, bool> experimentalFlags = parseExperimentalFlags(
parseExperimentalArguments(experiments),
@@ -89,6 +92,7 @@
// We'll load a new sdk, anything loaded already will have a wrong root.
workerInputCache.clear();
+ workerInputCacheLibs.clear();
} else {
// We do have a previous state.
cachedSdkInput = workerInputCache[sdkSummary];
@@ -98,112 +102,161 @@
startOver = true;
// We'll load a new sdk, anything loaded already will have a wrong root.
workerInputCache.clear();
+ workerInputCacheLibs.clear();
}
}
- if (startOver) {
- // The sdk was either not cached or it has changed.
- options = new CompilerOptions()
- ..sdkSummary = sdkSummary
- ..packagesFileUri = packagesFile
- ..librariesSpecificationUri = librariesSpecificationUri
- ..target = target
- ..fileSystem = fileSystem
- ..omitPlatform = true
- ..environmentDefines = const {}
- ..experimentalFlags = experimentalFlags;
-
- processedOpts = new ProcessedOptions(options: options);
- cachedSdkInput = WorkerInputComponent(
- sdkDigest, await processedOpts.loadSdkSummary(null));
- workerInputCache[sdkSummary] = cachedSdkInput;
-
- incrementalCompiler = new IncrementalCompiler.fromComponent(
- new CompilerContext(processedOpts),
- cachedSdkInput.component,
- outlineOnly);
- incrementalCompiler.trackNeededDillLibraries = trackNeededDillLibraries;
- } else {
- options = oldState.options;
- processedOpts = oldState.processedOpts;
- var sdkComponent = cachedSdkInput.component;
- // Reset the state of the component.
- for (var lib in sdkComponent.libraries) {
- lib.isExternal = cachedSdkInput.externalLibs.contains(lib.importUri);
- }
-
- // Make sure the canonical name root knows about the sdk - otherwise we
- // won't be able to link to it when loading more outlines.
- sdkComponent.adoptChildren();
-
- // TODO(jensj): This is - at least currently - necessary,
- // although it's not entirely obvious why.
- // It likely has to do with several outlines containing the same libraries.
- // Once that stops (and we check for it) we can probably remove this,
- // and instead only do it when about to reuse an outline in the
- // 'inputSummaries.add(component);' line further down.
- for (WorkerInputComponent cachedInput in workerInputCache.values) {
- cachedInput.component.adoptChildren();
- }
-
- // Reuse the incremental compiler, but reset as needed.
- incrementalCompiler = oldState.incrementalCompiler;
- incrementalCompiler.invalidateAllSources();
- incrementalCompiler.trackNeededDillLibraries = trackNeededDillLibraries;
- options.packagesFileUri = packagesFile;
- options.fileSystem = fileSystem;
- processedOpts.clearFileSystemCache();
- }
-
- // Then read all the input summary components.
- CanonicalName nameRoot = cachedSdkInput.component.root;
- final inputSummaries = <Component>[];
+ bool retry = true;
+ int tryCount = 0;
+ final List<Component> inputSummaries = <Component>[];
Map<Uri, Uri> libraryToInputDill;
- if (trackNeededDillLibraries) {
- libraryToInputDill = new Map<Uri, Uri>();
- }
- List<Uri> loadFromDill = new List<Uri>();
- for (Uri summary in summaryInputs) {
- var cachedInput = workerInputCache[summary];
- var summaryDigest = workerInputDigests[summary];
- if (summaryDigest == null) {
- throw new StateError("Expected to get digest for $summary");
- }
- if (cachedInput == null ||
- cachedInput.component.root != nameRoot ||
- !digestsEqual(cachedInput.digest, summaryDigest)) {
- loadFromDill.add(summary);
+ doRetry:
+ while (retry) {
+ retry = false;
+ tryCount++;
+ if (startOver) {
+ // The sdk was either not cached or it has changed.
+ options = new CompilerOptions()
+ ..sdkSummary = sdkSummary
+ ..packagesFileUri = packagesFile
+ ..librariesSpecificationUri = librariesSpecificationUri
+ ..target = target
+ ..fileSystem = fileSystem
+ ..omitPlatform = true
+ ..environmentDefines = const {}
+ ..experimentalFlags = experimentalFlags;
+
+ processedOpts = new ProcessedOptions(options: options);
+ cachedSdkInput = WorkerInputComponent(
+ sdkDigest, await processedOpts.loadSdkSummary(null));
+ workerInputCache[sdkSummary] = cachedSdkInput;
+ for (Library lib in cachedSdkInput.component.libraries) {
+ if (workerInputCacheLibs.containsKey(lib.importUri)) {
+ throw new StateError("Duplicate sources in sdk.");
+ }
+ workerInputCacheLibs[lib.importUri] = sdkSummary;
+ }
+
+ incrementalCompiler = new IncrementalCompiler.fromComponent(
+ new CompilerContext(processedOpts),
+ cachedSdkInput.component,
+ outlineOnly);
+ incrementalCompiler.trackNeededDillLibraries = trackNeededDillLibraries;
} else {
- // Need to reset cached components so they are usable again.
- var component = cachedInput.component;
- for (var lib in component.libraries) {
- lib.isExternal = cachedInput.externalLibs.contains(lib.importUri);
+ options = oldState.options;
+ processedOpts = oldState.processedOpts;
+ var sdkComponent = cachedSdkInput.component;
+ // Reset the state of the component.
+ for (var lib in sdkComponent.libraries) {
+ lib.isExternal = cachedSdkInput.externalLibs.contains(lib.importUri);
+ }
+
+ // Make sure the canonical name root knows about the sdk - otherwise we
+ // won't be able to link to it when loading more outlines.
+ sdkComponent.adoptChildren();
+
+ // TODO(jensj): This is - at least currently - necessary,
+ // although it's not entirely obvious why.
+ // It likely has to do with several outlines containing the same libraries.
+ // Once that stops (and we check for it) we can probably remove this,
+ // and instead only do it when about to reuse an outline in the
+ // 'inputSummaries.add(component);' line further down.
+ for (WorkerInputComponent cachedInput in workerInputCache.values) {
+ cachedInput.component.adoptChildren();
+ }
+
+ // Reuse the incremental compiler, but reset as needed.
+ incrementalCompiler = oldState.incrementalCompiler;
+ incrementalCompiler.invalidateAllSources();
+ incrementalCompiler.trackNeededDillLibraries = trackNeededDillLibraries;
+ options.packagesFileUri = packagesFile;
+ options.fileSystem = fileSystem;
+ processedOpts.clearFileSystemCache();
+ }
+
+ // Then read all the input summary components.
+ CanonicalName nameRoot = cachedSdkInput.component.root;
+ final inputSummaries = <Component>[];
+ if (trackNeededDillLibraries) {
+ libraryToInputDill = new Map<Uri, Uri>();
+ }
+ List<Uri> loadFromDill = new List<Uri>();
+ for (Uri summary in summaryInputs) {
+ var cachedInput = workerInputCache[summary];
+ var summaryDigest = workerInputDigests[summary];
+ if (summaryDigest == null) {
+ throw new StateError("Expected to get digest for $summary");
+ }
+ if (cachedInput == null ||
+ cachedInput.component.root != nameRoot ||
+ !digestsEqual(cachedInput.digest, summaryDigest)) {
+ // Remove any old libraries from workerInputCacheLibs.
+ Component component = cachedInput?.component;
+ if (component != null) {
+ for (Library lib in component.libraries) {
+ workerInputCacheLibs.remove(lib.importUri);
+ }
+ }
+ loadFromDill.add(summary);
+ } else {
+ // Need to reset cached components so they are usable again.
+ var component = cachedInput.component;
+ for (var lib in component.libraries) {
+ lib.isExternal = cachedInput.externalLibs.contains(lib.importUri);
+ if (trackNeededDillLibraries) {
+ libraryToInputDill[lib.importUri] = summary;
+ }
+ }
+ inputSummaries.add(component);
+ }
+ }
+
+ for (int i = 0; i < loadFromDill.length; i++) {
+ Uri summary = loadFromDill[i];
+ List<int> summaryDigest = workerInputDigests[summary];
+ if (summaryDigest == null) {
+ throw new StateError("Expected to get digest for $summary");
+ }
+ WorkerInputComponent cachedInput = WorkerInputComponent(
+ summaryDigest,
+ await processedOpts.loadComponent(
+ await fileSystem.entityForUri(summary).readAsBytes(), nameRoot,
+ alwaysCreateNewNamedNodes: true));
+ workerInputCache[summary] = cachedInput;
+ inputSummaries.add(cachedInput.component);
+
+ for (var lib in cachedInput.component.libraries) {
if (trackNeededDillLibraries) {
libraryToInputDill[lib.importUri] = summary;
}
+
+ if (workerInputCacheLibs.containsKey(lib.importUri)) {
+ // We now have the same library twice. That doesn't work.
+ // Throw away previous state and reload. If we tried that already,
+ // throw an error.
+ print("Trying again because of ${lib.importUri} in both $summary and "
+ "${workerInputCacheLibs[lib.importUri]}");
+ startOver = true;
+ workerInputCache.clear();
+ workerInputCacheLibs.clear();
+ inputSummaries.clear();
+ retry = true;
+ if (tryCount > 1) {
+ throw new StateError("Loading the same library more than once: "
+ "${lib.importUri} (from both $summary and "
+ "${workerInputCacheLibs[lib.importUri]}");
+ }
+ continue doRetry;
+ } else {
+ workerInputCacheLibs[lib.importUri] = summary;
+ }
}
- inputSummaries.add(component);
}
}
- for (int i = 0; i < loadFromDill.length; i++) {
- Uri summary = loadFromDill[i];
- List<int> summaryDigest = workerInputDigests[summary];
- if (summaryDigest == null) {
- throw new StateError("Expected to get digest for $summary");
- }
- WorkerInputComponent cachedInput = WorkerInputComponent(
- summaryDigest,
- await processedOpts.loadComponent(
- await fileSystem.entityForUri(summary).readAsBytes(), nameRoot,
- alwaysCreateNewNamedNodes: true));
- workerInputCache[summary] = cachedInput;
- inputSummaries.add(cachedInput.component);
- if (trackNeededDillLibraries) {
- for (var lib in cachedInput.component.libraries) {
- libraryToInputDill[lib.importUri] = summary;
- }
- }
+ if (tryCount > 1) {
+ print("This build will take longer because of a retry caused "
+ "by duplicate sources.");
}
incrementalCompiler.setModulesToLoadOnNextComputeDelta(inputSummaries);
diff --git a/pkg/front_end/lib/src/api_unstable/compiler_state.dart b/pkg/front_end/lib/src/api_unstable/compiler_state.dart
index 57fdac4..b71e597 100644
--- a/pkg/front_end/lib/src/api_unstable/compiler_state.dart
+++ b/pkg/front_end/lib/src/api_unstable/compiler_state.dart
@@ -15,11 +15,13 @@
final CompilerOptions options;
final ProcessedOptions processedOpts;
final Map<Uri, WorkerInputComponent> workerInputCache;
+ final Map<Uri, Uri> workerInputCacheLibs;
final IncrementalCompiler incrementalCompiler;
final Map<Uri, Uri> libraryToInputDill;
InitializedCompilerState(this.options, this.processedOpts,
{this.workerInputCache,
+ this.workerInputCacheLibs,
this.incrementalCompiler,
this.libraryToInputDill});
}
To view, visit change 113023. To unsubscribe, or for help writing mail filters, visit settings.
Jake Macdonald (Gerrit)
Aug 14, 2019, 10:14:11 AM8/14/19
to Jens Johansen, dart-fe-te...@google.com, rev...@dartlang.org, commi...@chromium.org
Do we actually need to land this? It seems like it would be likely to mask over real problems.
Duplicate sources internally are all but cleaned up at this point (just a single cl remaining).
1 comment:
File pkg/front_end/lib/src/api_unstable/bazel_worker.dart:
Patch Set #3, Line 113:
doRetry:Haha what I had no idea dart even had this feature. This looks effectively like goto?
To view, visit change 113023. To unsubscribe, or for help writing mail filters, visit settings.
Jake Macdonald (Gerrit)
Aug 14, 2019, 10:24:19 AM8/14/19
to Jens Johansen, dart-fe-te...@google.com, rev...@dartlang.org, commi...@chromium.org
Oh sorry I just saw you had a similar comment, I really hate gerrit lol.
Anyways, ya unless you thing this is absolutely necessary I would not land it personally.
Jens Johansen (Gerrit)
Jun 29, 2022, 4:02:43 AM6/29/22
to dart-fe-te...@google.com, rev...@dartlang.org, Jake Macdonald, Commit Bot
Jens Johansen abandoned this change.
Abandoned
To view, visit change 113023. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages