Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 5, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f5f558
Add to `resolver_test`, add to `AnalysisDriverModel` as needed to sat…
davidmorgan Feb 3, 2025
3cf51b3
Remove unneeded null check.
davidmorgan Feb 4, 2025
64dc3c5
Address review comments.
davidmorgan Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 185 additions & 52 deletions build_resolvers/lib/src/analysis_driver_model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,24 @@ class AnalysisDriverModel {
final MemoryResourceProvider resourceProvider =
MemoryResourceProvider(context: p.posix);

/// The import graph of all sources needed for analysis.
final _graph = _Graph();

/// Assets that have been synced into the in-memory filesystem
/// [resourceProvider].
final _syncedOntoResourceProvider = <AssetId>{};

/// Notifies that [step] has completed.
///
/// All build steps must complete before [reset] is called.
void notifyComplete(BuildStep step) {
// TODO(davidmorgan): add test coverage, fix implementation.
// This implementation doesn't keep state per `BuildStep`, nothing to do.
}

/// Clear cached information specific to an individual build.
void reset() {
// TODO(davidmorgan): add test coverage, fix implementation.
_graph.clear();
_syncedOntoResourceProvider.clear();
}

/// Attempts to parse [uri] into an [AssetId] and returns it if it is cached.
Expand Down Expand Up @@ -82,66 +90,85 @@ class AnalysisDriverModel {
FutureOr<void> Function(AnalysisDriverForPackageBuild))
withDriverResource,
{required bool transitive}) async {
/// TODO(davidmorgan): add test coverage for whether transitive
/// sources are read when [transitive] is false, fix the implementation
/// here.
/// TODO(davidmorgan): add test coverage for whether
/// `.transitive_deps` files cut off the reporting of deps to the
/// [buildStep], fix the implementation here.

// Find transitive deps, this also informs [buildStep] of all inputs).
final ids = await _expandToTransitive(buildStep, entryPoints);

// Apply changes to in-memory filesystem.
for (final id in ids) {
if (await buildStep.canRead(id)) {
final content = await buildStep.readAsString(id);

/// TODO(davidmorgan): add test coverage for when a file is
/// modified rather than added, fix the implementation here.
resourceProvider.newFile(id.asPath, content);
} else {
if (resourceProvider.getFile(id.asPath).exists) {
resourceProvider.deleteFile(id.asPath);
}
}
}

// Notify the analyzer of changes.
// Immediately take the lock on `driver` so that the whole class state,
// `_graph` and `_readForAnalyzir`, is only mutated by one build step at a
// time. Otherwise, interleaved access complicates processing significantly.
await withDriverResource((driver) async {
for (final id in ids) {
// TODO(davidmorgan): add test coverage for over-notification of
// changes, fix the implementaion here.
driver.changeFile(id.asPath);
}
await driver.applyPendingFileChanges();
return _performResolve(driver, buildStep, entryPoints, withDriverResource,
transitive: transitive);
});
}

/// Walks the import graph from [ids], returns full transitive deps.
Future<Set<AssetId>> _expandToTransitive(
AssetReader reader, Iterable<AssetId> ids) async {
final result = ids.toSet();
final nextIds = Queue.of(ids);
while (nextIds.isNotEmpty) {
final nextId = nextIds.removeFirst();
Future<void> _performResolve(
AnalysisDriverForPackageBuild driver,
BuildStep buildStep,
List<AssetId> entryPoints,
Future<void> Function(
FutureOr<void> Function(AnalysisDriverForPackageBuild))
withDriverResource,
{required bool transitive}) async {
var idsToSyncOntoResourceProvider = entryPoints;
Iterable<AssetId> inputIds = entryPoints;

// Skip if not readable. Note that calling `canRead` still makes it a
// dependency of the `BuildStep`.
if (!await reader.canRead(nextId)) continue;
// If requested, find transitive imports.
if (transitive) {
await _graph.load(buildStep, entryPoints);
idsToSyncOntoResourceProvider = _graph.nodes.keys.toList();
inputIds = _graph.inputsFor(entryPoints);

final content = await reader.readAsString(nextId);
final deps = _parseDependencies(content, nextId);
// Check for inputs that were missing when the directive graph was read
// but have since been written by another build action.
for (final id in inputIds
.where((id) => !id.path.endsWith(_transitiveDigestExtension))) {
if (_graph.nodes[id]!.isMissing) {
if (await buildStep.canRead(id)) {
idsToSyncOntoResourceProvider.add(id);
_syncedOntoResourceProvider.remove(id);
}
}
}
}

// For each dep, if it's not in `result` yet, it's newly-discovered:
// add it to `nextIds`.
for (final dep in deps) {
if (result.add(dep)) {
nextIds.add(dep);
// Notify [buildStep] of its inputs.
for (final id in inputIds) {
await buildStep.canRead(id);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Sync changes onto the "URI resolver", the in-memory filesystem.
final changedIds = <AssetId>[];
for (final id in idsToSyncOntoResourceProvider) {
if (!_syncedOntoResourceProvider.add(id)) continue;
final content =
await buildStep.canRead(id) ? await buildStep.readAsString(id) : null;
final inMemoryFile = resourceProvider.getFile(id.asPath);
final inMemoryContent =
inMemoryFile.exists ? inMemoryFile.readAsStringSync() : null;

if (content != inMemoryContent) {
if (content == null) {
// TODO(davidmorgan): per "globallySeenAssets" in
// BuildAssetUriResolver, deletes should only be applied at the end
// of the build, in case the file is actually there but not visible
// to the current reader.
resourceProvider.deleteFile(id.asPath);
changedIds.add(id);
} else {
if (inMemoryContent == null) {
resourceProvider.newFile(id.asPath, content);
} else {
resourceProvider.modifyFile(id.asPath, content);
}
changedIds.add(id);
}
}
}
return result;

// Notify the analyzer of changes and wait for it to update its internal
// state.
for (final id in changedIds) {
driver.changeFile(id.asPath);
}
await driver.applyPendingFileChanges();
}
}

Expand All @@ -167,3 +194,109 @@ extension _AssetIdExtensions on AssetId {
/// Asset path for the in-memory filesystem.
String get asPath => AnalysisDriverModelUriResolver.assetPath(this);
}

/// The directive graph of all known sources.
///
/// Also tracks whether there is a `.transitive_digest` file next to each source
/// asset, and tracks missing files.
class _Graph {
final Map<AssetId, _Node> nodes = {};

/// Walks the import graph from [ids] loading into [nodes].
Future<void> load(AssetReader reader, Iterable<AssetId> ids) async {
// TODO(davidmorgan): check if List is faster.
final nextIds = Queue.of(ids);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

while (nextIds.isNotEmpty) {
final nextId = nextIds.removeFirst();

// Skip if already seen.
if (nodes.containsKey(nextId)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


final hasTransitiveDigestAsset =
await reader.canRead(nextId.addExtension(_transitiveDigestExtension));

// Skip if not readable.
if (!await reader.canRead(nextId)) {
nodes[nextId] = _Node.missing(
id: nextId, hasTransitiveDigestAsset: hasTransitiveDigestAsset);
continue;
}

final content = await reader.readAsString(nextId);
final deps = _parseDependencies(content, nextId);
nodes[nextId] = _Node(
id: nextId,
deps: deps,
hasTransitiveDigestAsset: hasTransitiveDigestAsset);
nextIds.addAll(deps.where((id) => !nodes.containsKey(id)));
}
}

void clear() {
nodes.clear();
}

/// The inputs for a build action analyzing [entryPoints].
///
/// This is transitive deps, but cut off by the presence of any
/// `.transitive_digest` file next to an asset.
Set<AssetId> inputsFor(Iterable<AssetId> entryPoints) {
final result = entryPoints.toSet();
final nextIds = Queue.of(entryPoints);

while (nextIds.isNotEmpty) {
final nextId = nextIds.removeFirst();
final node = nodes[nextId]!;

// Add the transitive digest file as an input. If it exists, skip deps.
result.add(nextId.addExtension(_transitiveDigestExtension));
if (node.hasTransitiveDigestAsset) {
continue;
}

// Skip if there are no deps because the file is missing.
if (node.isMissing) continue;

// For each dep, if it's not in `result` yet, it's newly-discovered:
// add it to `nextIds`.
for (final dep in node.deps) {
if (result.add(dep)) {
nextIds.add(dep);
}
}
}
return result;
}

@override
String toString() => nodes.toString();
}

/// A node in the directive graph.
class _Node {
final AssetId id;
final List<AssetId> deps;
final bool isMissing;
final bool hasTransitiveDigestAsset;

_Node(
{required this.id,
required this.deps,
required this.hasTransitiveDigestAsset})
: isMissing = false;

_Node.missing({required this.id, required this.hasTransitiveDigestAsset})
: isMissing = true,
deps = const [];

@override
String toString() => '$id:'
'${hasTransitiveDigestAsset ? 'digest:' : ''}'
'${isMissing ? 'missing' : deps}';
}

// Transitive digest files are built next to source inputs. As the name
// suggests, they contain the transitive digest of all deps of the file.
// So, establishing a dependency on a transitive digest file is equivalent
// to establishing a dependency on all deps of the file.
const _transitiveDigestExtension = '.transitive_digest';
Loading
Loading