Incremental CFE error from DDK #36644
Comments
vsmenon
added
web-dev-compiler
area-front-end
|
fyi - @stefantsov @jmesserly @jakemac53 @natebosch I can trigger this reliably by running flutter web gallery with webdev alpha and repeated editing files. @kevmoo just hit similar. |
|
This seems to be an issue with how we're saving state? I can paper over this by tossing the lastResult: |
vsmenon
added
the
P1
|
Here's a cleaned up version of the above: https://dart-review.googlesource.com/c/sdk/+/99442 This prevents crashes with webdev - ddk, but we still need to understand the root cause. |
|
We'll land the above, but keep chasing on this. |
|
From @jakemac53 :
|
|
Jake and I made a lot of progress here. Here's what we found so far:
So we started investigating invalidation logic, and found that CFE has two files, ddc.dart and bazel_worker.dart. They both have a method called I'm trying to switch DDC over to the bazel_worker method, and see if that fixes the problem. (Another thing we're not sure about... if the chip_demo file is getting read from source. It shouldn't because it's from an input Kernel file. But that might explain why we get two copies of the same ChipDemo class.) |
A theory about this: when we have 1 worker, the jobs get sent in a consistent order. So that gives the incremental compiler a chance to see all of the changes consistently. With multiple workers, the compile jobs are getting parallelized, so the incremental compiler might "miss" an update. If we have a bug in the invalidation/state update logic, it might manifest only when the compile jobs are run in a particular order on a worker. |
|
The above is landed - that just hides the bug (reasonably well from testing, but still concerned about this). @jmesserly is chasing down a proper fix. |
|
Update: tried switching to the bazel_worker.dart initializeIncrementalCompiler (with small changes so DDC can pass in some options) but haven't gotten it working with DDC's backend yet. Still investigating. Update 2: got it working, but it doesn't fix the problem. I'll keep investigating. Update 3: investigated the "loadFromDill" list that @jakemac53 was trying to log earlier, and it does look correct: only chip_demo's Kernel file is marked for reload. So it seems some amount of reloading is happening, but we have a stale reference somewhere. My current theory is the ClassHierarchy update logic (see applyTreeChanges) isn't working, or is being run at the wrong time . Also, I can see now why this only fails in DDC and not the Kernel file builder: the failure is in "KernelTarget.buildComponent" (called from incremental_compiler.dart) which only runs if "outlineOnly" is false. bazel_worker.dart passes "true" for that. |
|
I have to leave to meet someone, but I'll pick this up later tonight. My best guess currently: it's something related to class hierarchy invalidation. When I added this print: @override
ClassHierarchy applyTreeChanges(Iterable<Library> removedLibraries,
Iterable<Library> ensureKnownLibraries,
{Component reissueAmbiguousSupertypesFor}) {
// Remove all references to the removed classes.
for (Library lib in removedLibraries) {
if (!knownLibraries.contains(lib)) {
print('NOT FOUND ${lib.importUri}');
continue;
}
// ...It thinks "package:flutter_web.examples.gallery/demo/material/chip_demo.dart" is not found. Maybe it's using the wrong Library object (the new instance, rather than the old one)? I wonder if it would find the Library if it looked it up by Uri, rather than by the instance. edit: the problem with this theory, is that it doesn't explain why we failed to add the correct new data. I saw it repopulate the class hierarchy with lots of libraries, including chip_demo. But that instance must've been out of date too, because it didn't match the version we later looked up during type inference. |
|
Update: still haven't tracked down the root cause. I've been able to eliminate several theories, based on experimental fixes that didn't work. Will look at it more tomorrow, unless someone tracks it down in the meantime. |
Workaround for #36644 See similar for dartdevc: https://dart-review.googlesource.com/c/sdk/+/99442 Change-Id: Id02549a7bd8110b4691a724fa8930ede477464f8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99735 Commit-Queue: Vijay Menon <vsm@google.com> Reviewed-by: Jake Macdonald <jakemac@google.com>
|
Some progress. Applying this patch: --- a/pkg/front_end/lib/src/fasta/source/source_loader.dart
+++ b/pkg/front_end/lib/src/fasta/source/source_loader.dart
@@ -802,6 +802,12 @@ class SourceLoader extends Loader<Library> {
for (LibraryDependency dependency in library.dependencies) {
if (libraries.add(dependency.targetLibrary)) {
workList.add(dependency.targetLibrary);
+
+ var target = this.read(dependency.targetLibrary.importUri, 0).target;
+ if (target != dependency.targetLibrary) {
+ print('LOOKUP FAIL: ${dependency.targetLibrary.importUri} '
+ 'found distinct library ${target.hashCode}, instead of ${dependency.targetLibrary.hashCode}');
+ }
}
}This prints that the lookup failed. This is the cause of the class hierarchy lookup failure, because we If we added "target" in the code above to the workList (rather than dependency.targetLibrary) that would most likely fix the ClassHierarchy lookup bug, but it doesn't explain why this data got corrupted in the first place. IncrementalCompiler appears to be designed to avoid this problem, by invalidating any downstream libraries that depend on an invalidated one. So we shouldn't be getting incorrect library dependencies. It looks like the "target" variable is coming from the dill file for chip_demo (which seems correct), but I'm not sure where the other one is coming from, or what library the import is from (presumably demos.dart). What could be going wrong: if we have leftover data from when we compiled "chip_demo" from source, maybe we're finding that rather than the data from the summary. I have to go now, but I'll keep digging into it. edit: even on the first successful compile, there are (at least) 6 distinct versions of chip_demo. All of them reached from an export in "flutter_web.examples.gallery/demo/material/material.dart" (which is itself exported from all.dart, and that is imported from demos.dart, the file that fails to compile). In the initial compile, the "lookup failures" start from things downstream from all.dart (including demos.dart, although it succeeds that time). On the failed compile, the "lookup failures" start from all.dart itself, and there are again at least 6 versions of chip_demo. |
Workaround for #36644 See similar for dartdevc: https://dart-review.googlesource.com/c/sdk/+/99442 Change-Id: Id02549a7bd8110b4691a724fa8930ede477464f8 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99735 Commit-Queue: Vijay Menon <vsm@google.com> Reviewed-by: Jake Macdonald <jakemac@google.com>
|
@kmillikin - can you or someone on your team please take a look? We've covered this up with bandaids for now, but the underlying issue remains worrisome. A real fix here may be worth cherry-picking into Dart 2.3 / Flutter. |
|
/cc @jensjoha |
|
This issue is due to Kernel canonical names. Canonical names are the way that we link Kernel modules together. The original implementation was that we built them just before serializing to a .dill file and we discarded them just after --- Kernel in-memory normally didn't have canonical names. We've switched to a model where we do have canonical names in memory (with the incremental compiler) and we transfer libraries between components, which entails transferring ownership of canonical name subtrees. So we have to be very careful about ownership of canonical names to ensure that we reuse already-created Kernel nodes when we need to and create new replacement nodes when we need to. CL 100380 should fix this problem. A related problem is that we have to be careful to build transformations so that they work with the incremental compiler. The Flutter widget inspector, for instance, transforms classes by adding a field to them. It needs to ensure that it correctly creates this new field to properly replace a previously-created field for the same class. A full fix for this issue is probably to provide a restricted API for third-party transformations. @jakemac53 encountered another issue with CL 100380 mentioned in the comments there that we have not been able to reproduce. |
When editing Flutter Web Gallery or similar, I occasionally get the following stack trace:
This appears to be unrelated to the constant flag. I'm hitting this in bleeding edge. @kevmoo has hit the same in dev (a few days old).
The text was updated successfully, but these errors were encountered: