Add to `resolver_test`, add to `AnalysisDriverModel` as needed to satisfy the tests. #3822

davidmorgan · 2025-02-04T09:26:15Z

Run the tests with both "shared" instances, re-used between tests (as before) and "new" instances.

The new implementation is still not used, and is not finished: it's a further step towards adding tests / simplifying to understand what's going on and what can be done to improve performance.

This version does already save some time on the big builds :)

skip-changelog-check

github-actions · 2025-02-04T09:27:14Z

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:build	2.4.2	already published at pub.dev
package:build_config	1.1.2	already published at pub.dev
package:build_daemon	4.0.3	already published at pub.dev
package:build_modules	5.0.11	already published at pub.dev
package:build_resolvers	2.4.4-wip	WIP (no publish necessary)
package:build_runner	2.4.15-wip	WIP (no publish necessary)
package:build_runner_core	8.0.1-dev	ready to publish	`build_runner_core-v8.0.1-dev`
package:build_test	2.2.4-wip	WIP (no publish necessary)
package:build_web_compilers	4.1.1	already published at pub.dev
package:scratch_space	1.0.3-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

github-actions · 2025-02-04T09:28:09Z

PR Health

…isfy the tests. Run the tests with both "shared" instances, re-used between tests (as before) and "new" instances.

jensjoha · 2025-02-04T12:59:20Z

build_resolvers/lib/src/analysis_driver_model.dart


-      final content = await reader.readAsString(nextId);
-      final deps = _parseDependencies(content, nextId);
+      // Check for missing inputs that were written during the build.


I don't understand what a "missing input" is.

Updated to

// Check for inputs that were missing when the directive graph was read // but have since been written by another build action.

jensjoha · 2025-02-04T13:01:04Z

build_resolvers/lib/src/analysis_driver_model.dart

-          nextIds.add(dep);
+    // Notify [buildStep] of its inputs.
+    for (final id in inputIds) {
+      await buildStep.canRead(id);


Does "canRead" say "this is what you should process" or does it say "this is what you are allowed to read" or how am I supposed to read the code in combination with the comment?

Also possibly some were already "canRead" above (in if (transitive)). Maybe this could be something like

if (transitive) { // bla bla for (final id in inputIds) { // Notify [buildStep] of its inputs. And also other stuff. if (await buildStep.canRead(id) && !id.path.endsWith(_transitiveDigestExtension) && _graph.nodes[id]!.isMissing) { // Some good comment here. idsToSyncOntoResourceProvider.add(id); _syncedOntoResourceProvider.remove(id); } } } else { // Notify [buildStep] of its inputs. for (final id in inputIds) { await buildStep.canRead(id); } }

canRead asks the reader if the file is readable (exists + is allowed to be read), and marks the file as an input to the buildStep. I need to untangle those two soon to make progress.

notifying about inputs needs to be separate from reading; we can't efficiently track inputs while passing them around one by one

there should be no need to eagerly check whether a file was written, build_runner can track which files get written

so I'll leave it separate as after those changes the reading and notifying will actually be separate :)

Thanks.

jensjoha · 2025-02-04T13:17:22Z

build_resolvers/lib/src/analysis_driver_model.dart

+
+  /// Walks the import graph from [ids] loading into [nodes].
+  Future<void> load(AssetReader reader, Iterable<AssetId> ids) async {
+    final nextIds = Queue.of(ids);


I would have commented about this in one of the previous reviews as well but forgot.
Is there any reason this is a Queue instead of a List? A List should do removeLast instead - so ordering would change -but otherwise I'd assume it was similar and probably perform better because it doesn't have to do casts and head and tail book keeping etc.

No particular reason; but any cost here should be O(n), I want to solve the O(n^2) issues before getting onto constant factor improvements ... added a TODO. Thanks.

davidmorgan

Thanks.

davidmorgan · 2025-02-04T14:45:39Z

build_resolvers/lib/src/analysis_driver_model.dart

-          nextIds.add(dep);
+    // Notify [buildStep] of its inputs.
+    for (final id in inputIds) {
+      await buildStep.canRead(id);


canRead asks the reader if the file is readable (exists + is allowed to be read), and marks the file as an input to the buildStep. I need to untangle those two soon to make progress.

notifying about inputs needs to be separate from reading; we can't efficiently track inputs while passing them around one by one

there should be no need to eagerly check whether a file was written, build_runner can track which files get written

so I'll leave it separate as after those changes the reading and notifying will actually be separate :)

Thanks.

davidmorgan · 2025-02-04T14:45:45Z

build_resolvers/lib/src/analysis_driver_model.dart


-      final content = await reader.readAsString(nextId);
-      final deps = _parseDependencies(content, nextId);
+      // Check for missing inputs that were written during the build.


Updated to

// Check for inputs that were missing when the directive graph was read // but have since been written by another build action.

davidmorgan · 2025-02-04T14:52:24Z

build_resolvers/lib/src/analysis_driver_model.dart

+
+  /// Walks the import graph from [ids] loading into [nodes].
+  Future<void> load(AssetReader reader, Iterable<AssetId> ids) async {
+    final nextIds = Queue.of(ids);


No particular reason; but any cost here should be O(n), I want to solve the O(n^2) issues before getting onto constant factor improvements ... added a TODO. Thanks.

jakemac53 · 2025-02-04T15:39:37Z

build_resolvers/lib/src/analysis_driver_model.dart

+      final nextId = nextIds.removeFirst();
+
+      // Skip if already seen.
+      if (nodes.containsKey(nextId)) continue;


As far as I can tell you will run into trouble here - you may have previously put a _Node.missing in for this node, and will not be able to resolve it later on.

This is a key driver of the performance issues - the fact that we have to be able to adapt to new files being added means we cannot (safely) cache globally the transitive deps.

Note that even within a single build step you will run into this problem potentially, they are allowed to add new files which are imported by something they just resolved, and then resolve those files.

Yes, probably new work is needed for files created during the build.

Missing files are currently checked again, but only to see if they now exist, the graph is not recomputed.

It passes all the tests, though :) so more tests are needed, too.

One question, is it the case that all files that are output by generators during the build are written by an AssetWriter? Then, that should be a way to catch new files efficiently, rather than checking explicitly every time.

Thanks.

Yes, all files will be written by an AssetWriter. So, you may be able to add in some extra lifecycle hooks, but it starts getting into pretty dubious territory.

There is lots of weirdness because builders do not run in order most of the time, so you cannot rely on any phased ordering (instead they run when their output is requested). So, files go in and out of being "visible" depending on which builder happens to be running.

In order for a given input to have a consistent set of inputs, you have to do the crawl each time from each builder. We could possibly do something though where we accept that the inputs are over declared sometimes (a build step might pick up the transitive deps of a file it can't actually read).

Also, once you have been able to read a library you can cache its direct dependencies, which I believe the old system did do. So, you may still be able to do some of this.

Makes sense, thanks. Anyway, next step, test coverage :)

Add to resolver_test, add to AnalysisDriverModel as needed to sat…

4f5f558

…isfy the tests. Run the tests with both "shared" instances, re-used between tests (as before) and "new" instances.

davidmorgan force-pushed the extend-analysis-driver-model-and-tests branch from 3700547 to 4f5f558 Compare February 4, 2025 10:09

davidmorgan requested review from jensjoha and jakemac53 February 4, 2025 10:09

davidmorgan added the skip-changelog-check label Feb 4, 2025

Remove unneeded null check.

3cf51b3

davidmorgan marked this pull request as ready for review February 4, 2025 12:22

davidmorgan mentioned this pull request Feb 4, 2025

Simpler in memory filesystem #3823

Merged

jensjoha approved these changes Feb 4, 2025

View reviewed changes

Address review comments.

64dc3c5

davidmorgan commented Feb 4, 2025

View reviewed changes

jakemac53 reviewed Feb 4, 2025

View reviewed changes

davidmorgan merged commit 5b59745 into dart-lang:master Feb 5, 2025
75 checks passed

davidmorgan deleted the extend-analysis-driver-model-and-tests branch February 5, 2025 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add to `resolver_test`, add to `AnalysisDriverModel` as needed to satisfy the tests. #3822

Add to `resolver_test`, add to `AnalysisDriverModel` as needed to satisfy the tests. #3822

davidmorgan commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025 •

edited

Loading

jensjoha Feb 4, 2025

davidmorgan Feb 4, 2025

jensjoha Feb 4, 2025

jensjoha Feb 4, 2025

davidmorgan Feb 4, 2025

jensjoha Feb 4, 2025

davidmorgan Feb 4, 2025

davidmorgan left a comment

davidmorgan Feb 4, 2025

davidmorgan Feb 4, 2025

davidmorgan Feb 4, 2025

jakemac53 Feb 4, 2025

jakemac53 Feb 4, 2025

davidmorgan Feb 4, 2025

jakemac53 Feb 4, 2025

davidmorgan Feb 5, 2025

Add to resolver_test, add to AnalysisDriverModel as needed to satisfy the tests. #3822

Add to resolver_test, add to AnalysisDriverModel as needed to satisfy the tests. #3822

Conversation

davidmorgan commented Feb 4, 2025 • edited Loading

github-actions bot commented Feb 4, 2025

Package publishing

github-actions bot commented Feb 4, 2025 • edited Loading

PR Health

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add to `resolver_test`, add to `AnalysisDriverModel` as needed to satisfy the tests. #3822

Add to `resolver_test`, add to `AnalysisDriverModel` as needed to satisfy the tests. #3822

davidmorgan commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading