[flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing#184528
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request optimizes the flutter pub get command by implementing a PubspecCache to reduce redundant file I/O and enabling concurrent processing of workspace packages using a Pool. It also refactors plugin discovery and platform-specific tooling regeneration to share these cached resources. Feedback suggests parallelizing the cache initialization and restoring error logging for malformed pubspec files.
| Future<PubspecCache> buildPubspecCache( | ||
| PackageConfig packageConfig, { | ||
| FileSystem? fileSystem, | ||
| }) async { | ||
| final FileSystem fs = fileSystem ?? globals.fs; | ||
| final cache = <String, YamlMap?>{}; | ||
| for (final Package package in packageConfig.packages) { | ||
| final key = package.root.toString(); | ||
| final File pubspecFile = fs.file(package.root.resolve('pubspec.yaml')); | ||
| if (!pubspecFile.existsSync()) { | ||
| cache[key] = null; | ||
| continue; | ||
| } | ||
| try { | ||
| final Object? parsed = loadYaml(await pubspecFile.readAsString()); | ||
| cache[key] = parsed is YamlMap ? parsed : null; | ||
| } on YamlException { | ||
| cache[key] = null; | ||
| } | ||
| } | ||
| return cache; | ||
| } |
There was a problem hiding this comment.
The buildPubspecCache function currently reads and parses pubspec.yaml files sequentially. In a large workspace with hundreds of packages, this creates a serial bottleneck before the concurrent processing of roots begins. Parallelizing these I/O operations would further improve performance.
Additionally, this implementation loses the error logging for YamlException that was present in the original _pluginFromPackage logic. Restoring this logging (and adding it for FileSystemException) ensures that malformed or unreadable pubspecs are still reported in the trace logs.
Future<PubspecCache> buildPubspecCache(
PackageConfig packageConfig, {
FileSystem? fileSystem,
}) async {
final FileSystem fs = fileSystem ?? globals.fs;
final Map<String, YamlMap?> cache = <String, YamlMap?>{};
await Future.wait(packageConfig.packages.map((Package package) async {
final String key = package.root.toString();
final File pubspecFile = fs.file(package.root.resolve('pubspec.yaml'));
if (!pubspecFile.existsSync()) {
cache[key] = null;
return;
}
try {
final String content = await pubspecFile.readAsString();
final Object? parsed = loadYaml(content);
cache[key] = parsed is YamlMap ? parsed : null;
} on YamlException catch (err) {
globals.printTrace('Failed to parse pubspec.yaml for ${package.name}: $err');
cache[key] = null;
} on FileSystemException catch (err) {
globals.printTrace('Failed to read pubspec.yaml for ${package.name}: $err');
cache[key] = null;
}
}));
return cache;
}There was a problem hiding this comment.
Future.wait over a thousands files will likely hit the file descriptor limit which is 256 by default. this will result as a crash
error handling updated, good suggestion
There was a problem hiding this comment.
Actually, consider using package:pool to allow capped concurrency. Like 16?
There was a problem hiding this comment.
Actually, consider using
package:poolto allow capped concurrency. Like 16?
Thanks for your suggestion, implemented concurrent pubspec.yaml readns in f5152cf
There was a problem hiding this comment.
There was a problem hiding this comment.
https://pub.dev/documentation/pool/latest/pool/Pool/forEach.html
Even better!
Implemented in both places instead of Future.wait :)
| expect(cache[withoutPubspec.root.toString()], isNull); | ||
| }); | ||
|
|
||
| test('cached data survives deletion of source files', () async { |
There was a problem hiding this comment.
This is the key optimization property: once the cache is built, the original files are no longer needed. findPlugins (and related functions) can use the in-memory map instead of re-reading from disk
| final pool = Pool(globals.platform.numberOfProcessors); | ||
| await Future.wait( |
There was a problem hiding this comment.
Not really sure if it's OK to relate on numberOfProcessors like that in here
The reason I've used this variable (instead of hard-coded value) here is that because inside the function we use considerable amount of processing power. So I think this kind of safety mechanism is a great solution to prevent pub get crashes on less powerful machines
But I also admit that this number might be increased 2 times (or hard-coded) and still there will be no crashes
6fbf469 to
610f26b
Compare
| final FileSystem fs = fileSystem ?? globals.fs; | ||
| YamlMap? pubspec; | ||
| if (pubspecCache != null) { | ||
| if (pubspecCache != null && pubspecCache.containsKey(packageRoot.toString())) { |
There was a problem hiding this comment.
pubspecCache is built from the root package's PackageConfig, which only contains packages in its transitive dependency graph. If a package is not in the cache, it means it's not a dependency of the root — for example, a plugin used only by an example app but not by the plugin itself. Without containsKey, a cache miss returns null, which is indistinguishable from "package has no pubspec.yaml", causing real plugins to be silently skipped in .flutter-plugins, that's why for such examples we have to re-read pubspec.yaml from disk once again
There was a problem hiding this comment.
This would be a great comment to be added in the code. I was curious if the distinction between "in the map but null" or "not in the map" is important.
| // Process workspace root packages concurrently, capped to | ||
| // numberOfProcessors to avoid exhausting file descriptors while still | ||
| // getting parallelism on multi-core machines. | ||
| await Pool(globals.platform.numberOfProcessors) |
There was a problem hiding this comment.
We likely can be MORE concurrent than the number of processors. A good thing to Google.
Thus says Gemini:
The short answer is: mapping to the number of CPU cores is a great default, but it isn't always the optimal ceiling for file operations. Whether you should tie your concurrency limit to the core count depends entirely on what your developer tool does with the file after it reads it.
Here is how to think about setting a reasonable number based on your tool's bottleneck.
1. When to Map to CPU Cores (CPU-Bound Tools)
If your tool reads a file and then immediately performs heavy processing—like a linter analyzing an AST, a compiler transforming code, or a bundler minifying assets—your bottleneck is the CPU, not the disk.
- The Heuristic: Use
Number of Cores(orCores - 1to keep the developer's OS responsive). - Why: If you read 32 files concurrently but only have 8 cores, you will just end up queueing 24 files in memory waiting for a CPU thread (or Isolate, if you're working in Dart) to become available. This spikes memory usage without speeding up the overall process.
2. When to Exceed CPU Cores (I/O-Bound Tools)
If your tool is simply reading files to hash them, copy them, or do a very quick regex search, your bottleneck is the storage drive. Modern NVMe SSDs are incredibly fast and use Native Command Queuing (NCQ) to handle multiple I/O requests in parallel.
- The Heuristic: Use a multiple of cores (e.g.,
Cores * 2orCores * 4), or a fixed cap between16and64. - Why: An idle CPU core can easily issue multiple read requests to an SSD and wait for the hardware interrupts. Tying an I/O-bound task strictly to a small core count (like 4 or 8) will underutilize the parallel read capabilities of modern drives.
3. The Danger Zone: File Descriptor Limits
The biggest risk of cranking up concurrent reads for a developer tool isn't usually CPU thrashing; it's crashing the tool by exhausting the operating system's file descriptors.
Operating systems strictly limit how many files a single process can have open simultaneously. On many macOS and Linux systems, the default soft limit is often 1024 or even 256. If you set your concurrency to 256 or higher to crawl a massive repository, your tool will likely throw an EMFILE (Too many open files) error.
For a bulletproof developer tool, staying under a concurrency of 64 to 128 is usually the sweet spot to saturate I/O without risking OS-level file descriptor limits.
There was a problem hiding this comment.
Good point, bumped the cap to 32 in d6c584c, I think it's the safest option here to make sure pub get will not crash on less powerful machines
There was a problem hiding this comment.
noticed that there is a limit of 64 threads in the bundle_builder:
so decided to increase the cap to 64 in the c038687 on my machine runs fine
There was a problem hiding this comment.
Tested this on 2-core CPU machine also, works perfectly fine
There was a problem hiding this comment.
Out of curiosity, do we know what are the performance gain from the two parallelization in this PR?
There was a problem hiding this comment.
I didn't benchmark this formally, but tested it in my production application
Before the PR: 8-10 minutes
Sequential file reads: around 2 minutes
With 2 threads from the pool everywhere: around 75 seconds
With 64 threads from the pool everywhere: around 40 seconds
Seems like not that much of improvement but still nice-to-have change
bkonyi
left a comment
There was a problem hiding this comment.
This seems reasonable, so LGTM! We'll need a second pair of eyes on this before it can land though.
chingjun
left a comment
There was a problem hiding this comment.
LGTM with nits in the comments.
| final FileSystem fs = fileSystem ?? globals.fs; | ||
| YamlMap? pubspec; | ||
| if (pubspecCache != null) { | ||
| if (pubspecCache != null && pubspecCache.containsKey(packageRoot.toString())) { |
There was a problem hiding this comment.
This would be a great comment to be added in the code. I was curious if the distinction between "in the map but null" or "not in the map" is important.
| // Process workspace root packages concurrently, capped to | ||
| // numberOfProcessors to avoid exhausting file descriptors while still | ||
| // getting parallelism on multi-core machines. | ||
| await Pool(globals.platform.numberOfProcessors) |
There was a problem hiding this comment.
Out of curiosity, do we know what are the performance gain from the two parallelization in this PR?
|
autosubmit label was removed for flutter/flutter/184528, because - The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Very sorry to bother, messed up with formatting a bit :( Now everything should be fine |
…PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528)
…PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528)
flutter/flutter@2fa45e0...c1b14e9 2026-04-14 15619084+vashworth@users.noreply.github.com Rebuild flutter tool skill (flutter/flutter#184975) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0851d988db03 to 4c382df6a25a (1 revision) (flutter/flutter#185025) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 5504504b38c2 to ee5afcef0596 (1 revision) (flutter/flutter#185024) 2026-04-14 dacoharkes@google.com [ci] Split up integration.shard record_use_test.dart (flutter/flutter#185022) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from d34c84df4c37 to 0851d988db03 (3 revisions) (flutter/flutter#185012) 2026-04-14 dacoharkes@google.com [record_use] Add recorded uses to link hooks (flutter/flutter#184869) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0e98a9c635bb to d34c84df4c37 (5 revisions) (flutter/flutter#185009) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from ef28089d6533 to 5504504b38c2 (3 revisions) (flutter/flutter#185008) 2026-04-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from K_2AkZL3Drs6cGE1q... to rB8LAuZL_DwHMssTU... (flutter/flutter#185007) 2026-04-14 rmacnak@google.com [fuchsia] Replace ambient-replace-as-executable with VmexResource. (flutter/flutter#184967) 2026-04-13 chinmaygarde@google.com [Impeller] Commands that don't specify their own viewports get the viewport of the render pass. (flutter/flutter#177473) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from bc1df263ff3f to 0e98a9c635bb (1 revision) (flutter/flutter#184995) 2026-04-13 737941+loic-sharma@users.noreply.github.com Update autosubmit guide with the emergency label (flutter/flutter#184993) 2026-04-13 69043738+aNOOBisTheGod@users.noreply.github.com [flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528) 2026-04-13 15619084+vashworth@users.noreply.github.com Fix codesign verification test for SwiftPM Add to App (flutter/flutter#184980) 2026-04-13 87962825+kyungilcho@users.noreply.github.com Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs (flutter/flutter#183555) 2026-04-13 15619084+vashworth@users.noreply.github.com Reland "Disable async mode with LLDB" (flutter/flutter#184970) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from 55ddd6bb8be5 to bc1df263ff3f (6 revisions) (flutter/flutter#184968) 2026-04-13 matej.knopp@gmail.com Expose platform specific handles for multi-window API (flutter/flutter#184662) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC boetger@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…fig across workspace packages during pub get post-processing (flutter#184528) In large pub workspaces, `flutter pub get` post-processing was reading `pubspec.yaml` files redundantly — once per transitive dependency per workspace root package (O(N×deps) disk reads). On a repo with 500+ plugin packages this caused `flutter pub get` to take ~10 minutes. This PR fixes that with three changes: **1. PubspecCache** — `buildPubspecCache()` reads every package's `pubspec.yaml` once and caches it by root URI. All workspace root packages share this cache during `findPlugins` / `computeTransitiveDependencies`, reducing pubspec reads from O(N×deps) to O(deps). **2. Shared PackageGraph and PackageConfig** — both are loaded once from the workspace root and passed down through `regeneratePlatformSpecificTooling` → `refreshPluginsList` / `injectPlugins` → `findPlugins` / `computeTransitiveDependencies`. A `useSharedResources` guard ensures shared caches are only used when the project is actually a member of the workspace graph (non-workspace projects and example apps that are not workspace roots fall back to loading their own resources). **3. Pool-based concurrency** — workspace root packages are processed concurrently using `package:pool`, capped to `platform.numberOfProcessors`, matching the pattern in `build_system.dart`. Tested changes on the https://github.com/aNOOBisTheGod/too-long-post-pub-get Before: 40-60 seconds After: ~8 seconds Also tested on our production application Before: 6-10 mins After: ~40 seconds Fixes flutter#184515
…fig across workspace packages during pub get post-processing (flutter#184528) In large pub workspaces, `flutter pub get` post-processing was reading `pubspec.yaml` files redundantly — once per transitive dependency per workspace root package (O(N×deps) disk reads). On a repo with 500+ plugin packages this caused `flutter pub get` to take ~10 minutes. This PR fixes that with three changes: **1. PubspecCache** — `buildPubspecCache()` reads every package's `pubspec.yaml` once and caches it by root URI. All workspace root packages share this cache during `findPlugins` / `computeTransitiveDependencies`, reducing pubspec reads from O(N×deps) to O(deps). **2. Shared PackageGraph and PackageConfig** — both are loaded once from the workspace root and passed down through `regeneratePlatformSpecificTooling` → `refreshPluginsList` / `injectPlugins` → `findPlugins` / `computeTransitiveDependencies`. A `useSharedResources` guard ensures shared caches are only used when the project is actually a member of the workspace graph (non-workspace projects and example apps that are not workspace roots fall back to loading their own resources). **3. Pool-based concurrency** — workspace root packages are processed concurrently using `package:pool`, capped to `platform.numberOfProcessors`, matching the pattern in `build_system.dart`. Tested changes on the https://github.com/aNOOBisTheGod/too-long-post-pub-get Before: 40-60 seconds After: ~8 seconds Also tested on our production application Before: 6-10 mins After: ~40 seconds Fixes flutter#184515
…ckageConfig across workspace packages during pub get post-processing (flutter#184528)" This reverts commit 2e3b7c0.
In large pub workspaces,
flutter pub getpost-processing was readingpubspec.yamlfiles redundantly — once per transitive dependency perworkspace root package (O(N×deps) disk reads). On a repo with 500+
plugin packages this caused
flutter pub getto take ~10 minutes.This PR fixes that with three changes:
1. PubspecCache —
buildPubspecCache()reads every package'spubspec.yamlonce and caches it by root URI. All workspace rootpackages share this cache during
findPlugins/computeTransitiveDependencies,reducing pubspec reads from O(N×deps) to O(deps).
2. Shared PackageGraph and PackageConfig — both are loaded once
from the workspace root and passed down through
regeneratePlatformSpecificTooling→
refreshPluginsList/injectPlugins→findPlugins/computeTransitiveDependencies. AuseSharedResourcesguard ensuresshared caches are only used when the project is actually a member of
the workspace graph (non-workspace projects and example apps that are
not workspace roots fall back to loading their own resources).
3. Pool-based concurrency — workspace root packages are processed
concurrently using
package:pool, capped toplatform.numberOfProcessors,matching the pattern in
build_system.dart.Tested changes on the https://github.com/aNOOBisTheGod/too-long-post-pub-get
Before: 40-60 seconds
After: ~8 seconds
Also tested on our production application
Before: 6-10 mins
After: ~40 seconds
Fixes #184515