Add new code for resolving deps graphs in a phased build. #3953

davidmorgan · 2025-03-28T10:48:11Z

The current code computes transitive deps for each build step using some cached intermediate results; this new code uses information about what phase generated files were or will be created in to compute everything once.

Two further steps after this PR to get the performance gains:

InputTracker and so AssetGraph currently flatten the computed LibraryCycleGraphs to a new set of IDs for each build step; instead they should reuse the instances created.
serialization should store multiple references to the same LibraryCycleGraph once, i.e. dedupe by identity

I've started looking at those changes in #3955 and indeed the performance looks promising.

github-actions · 2025-03-28T10:50:05Z

PR Health

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

jensjoha · 2025-04-04T07:57:07Z

build/lib/src/library_cycle_graph/library_cycle_graph.dart

+  LibraryCycleGraph._();
+
+  /// All subgraphs in the graph, including the root.
+  Iterable<LibraryCycleGraph> get transitiveGraphs sync* {


Any reason this (and the method below) is sync*?
While I haven't checked I'll assume there's quite a bit of performance overhead.
(Also I personally don't like them).

I'm not sure yet if it needs to be fast, and I find sync* easier to write / less likely to have bugs.

The TODO below is about writing away from sync* in case it does need to be fast :)

If there is a good pattern I can follow for writing it without sync*, happy to learn + do that now :)

In this instance I think returning seenGraphs would result in the same thing. It's not using more ram (you already have the Set), I'm guessing you're losing some overhead and that your profile will be better at assigning stuff correctly. And then you can get rid of the sync* :)

Oh, that's much better. Thanks :)

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

jensjoha · 2025-04-04T08:53:06Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+      final idToLoad = idsToLoad.removeLast();
+
+      // Nothing to do if deps were already loaded, unless they were
+      // loaded and expire and [assetDepsLoader] can see a newer value.


I don't understand the part about seeing a newer value?

Changed to:

// Nothing to do if deps were already loaded, unless they expire and // [assetDepsLoader] is at a late enough phase to see the updated value.

jensjoha · 2025-04-04T08:55:36Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+        // Work through phases <= `upToPhase` at which graphs expire,
+        // so there are new values to compute.
+        if (_graphsToComputeByPhase.isEmpty) break;
+        phase = (_graphsToComputeByPhase.keys.toList()..sort()).first;


how many phases do we have?
Maybe we should have a helper that fetches the smallest value without creating a new list and sorting?

A moderate number: one per builder per package it runs on.

There is definitely one value in keys because of the check one line above, so actually this can be .reduce(min) which is much better :)

jensjoha · 2025-04-04T09:05:13Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+
+      // Build graphs from cycles.
+      final existingCycles =
+          _cycles.values


I'd prefer doing a normal loop instead of doing .where.map.toList (e.g. see dart-lang/sdk#53987)

Done.

Also, moved into _buildGraphs, since it's only needed there.

jensjoha · 2025-04-04T09:23:54Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+      // The graph expires at the earliest of its root cycle deps expirey phases
+      // and all its child graph expirey phases, compute the initial value for
+      // the root to be updated as childen are added.
+      var expiresAt = root.ids


This is completely unreadable.

Took another shot at it :)

// The graph expires at first phase in which any asset in the graph // expires. Start this calculation by finding the earliest expirey phase // of all assets in the graph root cycle. It will be updated for each // child graph below.

and extracted an earliestPhase which I can use with reduce, and in updating expiresAt below, I think that's clearer.

It's super annoying that I'm not able to put the comment on a specific range of code as I can in gerrit :/

What was - and still is - unreadable (to me) is the code:

var expiresAt = root.ids .map((id) => _assetDeps[id]!.expiringValueAt(phase: phase).expiresAt) .fold<int?>( null, (a, b) => a == null ? b : b == null ? null : min(a, b), );

And now Github is showing me different code :S

Addressed elsewhere :)

jensjoha · 2025-04-04T09:38:21Z

build/lib/src/library_cycle_graph/phased_value.dart

+/// For example, if `foo.dart` is a generated file generated in phase 3:
+///
+///  - the value of `foo.dart` at phase 0 is empty/missing;
+///  - that value expires at phase 3


This is a good explanation, but it seems to me that it doesn't so much "expire at" as "expire after"? (which is also why it's available at expire+1).

E.g. also below --- wouldn't

bool isExpiredAt({required int phase}) { return expiresAt != null && expiresAt! < phase; }

be easier to undestand as

bool isExpiredAt({required int phase}) { return expiresAfter != null && expiresAfter! < phase; }

? (e.g. With "at" it sounds - at least to me - as if <= should be used instead of <).

Yes, that's a nice improvement, thanks :) done.

build/lib/src/library_cycle_graph/phased_value.dart

davidmorgan

Thanks, please see if my changes adequately address things :)

davidmorgan · 2025-04-04T11:26:53Z

build/lib/src/library_cycle_graph/library_cycle_graph.dart

+  LibraryCycleGraph._();
+
+  /// All subgraphs in the graph, including the root.
+  Iterable<LibraryCycleGraph> get transitiveGraphs sync* {


I'm not sure yet if it needs to be fast, and I find sync* easier to write / less likely to have bugs.

The TODO below is about writing away from sync* in case it does need to be fast :)

If there is a good pattern I can follow for writing it without sync*, happy to learn + do that now :)

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

davidmorgan · 2025-04-04T11:31:12Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+      final idToLoad = idsToLoad.removeLast();
+
+      // Nothing to do if deps were already loaded, unless they were
+      // loaded and expire and [assetDepsLoader] can see a newer value.


Changed to:

// Nothing to do if deps were already loaded, unless they expire and // [assetDepsLoader] is at a late enough phase to see the updated value.

davidmorgan · 2025-04-04T11:44:39Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+        // Work through phases <= `upToPhase` at which graphs expire,
+        // so there are new values to compute.
+        if (_graphsToComputeByPhase.isEmpty) break;
+        phase = (_graphsToComputeByPhase.keys.toList()..sort()).first;


A moderate number: one per builder per package it runs on.

There is definitely one value in keys because of the check one line above, so actually this can be .reduce(min) which is much better :)

davidmorgan · 2025-04-04T11:48:12Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+
+      // Build graphs from cycles.
+      final existingCycles =
+          _cycles.values


Done.

Also, moved into _buildGraphs, since it's only needed there.

davidmorgan · 2025-04-04T11:56:08Z

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

+      // The graph expires at the earliest of its root cycle deps expirey phases
+      // and all its child graph expirey phases, compute the initial value for
+      // the root to be updated as childen are added.
+      var expiresAt = root.ids


Took another shot at it :)

// The graph expires at first phase in which any asset in the graph // expires. Start this calculation by finding the earliest expirey phase // of all assets in the graph root cycle. It will be updated for each // child graph below.

and extracted an earliestPhase which I can use with reduce, and in updating expiresAt below, I think that's clearer.

davidmorgan · 2025-04-04T12:01:36Z

build/lib/src/library_cycle_graph/phased_value.dart

+/// For example, if `foo.dart` is a generated file generated in phase 3:
+///
+///  - the value of `foo.dart` at phase 0 is empty/missing;
+///  - that value expires at phase 3


Yes, that's a nice improvement, thanks :) done.

build/lib/src/library_cycle_graph/phased_value.dart

build/lib/src/library_cycle_graph/library_cycle_graph_loader.dart

jensjoha · 2025-04-04T12:31:09Z

build/lib/src/library_cycle_graph/library_cycle_graph.dart

+  LibraryCycleGraph._();
+
+  /// All subgraphs in the graph, including the root.
+  Iterable<LibraryCycleGraph> get transitiveGraphs sync* {


In this instance I think returning seenGraphs would result in the same thing. It's not using more ram (you already have the Set), I'm guessing you're losing some overhead and that your profile will be better at assigning stuff correctly. And then you can get rid of the sync* :)

davidmorgan force-pushed the deps-graph-with-phases-resolver-rebase branch 2 times, most recently from c970f30 to f8439e3 Compare March 31, 2025 11:26

davidmorgan changed the title ~~Deps graph with phases resolver rebase~~ Add new code for resolving deps graphs in a phased build. Mar 31, 2025

davidmorgan force-pushed the deps-graph-with-phases-resolver-rebase branch 4 times, most recently from 2e08485 to b44dd81 Compare April 2, 2025 15:27

Add new code for resolving deps graphs in a phased build.

4ecc64f

davidmorgan force-pushed the deps-graph-with-phases-resolver-rebase branch from b44dd81 to 4ecc64f Compare April 2, 2025 15:30

davidmorgan mentioned this pull request Apr 3, 2025

Input tracker WIP #3955

Closed

davidmorgan marked this pull request as ready for review April 3, 2025 12:15

davidmorgan requested a review from jensjoha April 3, 2025 12:15

jensjoha approved these changes Apr 4, 2025

View reviewed changes

Address review comments.

cadccf9

davidmorgan commented Apr 4, 2025

View reviewed changes

Address analyzer hint.

30e335b

jensjoha reviewed Apr 4, 2025

View reviewed changes

Address review comments.

cd64a3b

davidmorgan merged commit ec6d434 into dart-lang:master Apr 4, 2025
75 checks passed

Add new code for resolving deps graphs in a phased build. #3953

Add new code for resolving deps graphs in a phased build. #3953

Uh oh!

Conversation

davidmorgan commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Health

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidmorgan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidmorgan commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading