Skip to content

[native_assets_builder] The asset builder system should compile hook/{build,link}.dart once. #1578

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.

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

Open
mkustermann opened this issue Sep 18, 2024 · 1 comment
Labels
P3 A lower priority bug or feature request package:hooks_runner

Comments

@mkustermann
Copy link
Member

It seems a flutter build apk --release will compile the hook/build.dart script 3 times. Though a hook/build.dart compiled to build.dill doesn't depend on the build config and therefore shouldn't be in the build-config specific folder.

These are two separate concepts:

  • Caching of kernels of dart scripts to make hook invocations faster
  • Running build/link hooks with different build configurations

One could even argue that the native asset builder system should use dart run to invoke hooks (as the hook source code may transitively use assets on it's own) and the dart run tool should be responsible for optimizing repeated runs of the same script with same Dart SDK version.

But if we do it ourselves - we should avoid compiling it several times and putting it in a location that cannot be shared across runs.

@dcharkes
Copy link
Collaborator

@mosuem Suggested we could compile all hooks together to reduce the number of compilations.

(We could also reduce the amount of jit-compiling while running the hooks by generating a main function that runs all the hooks in topological order and produces all the build configs and build outputs, but that's a larger refactoring.)

One thing that we should take of though if we go that route is that hooks themselves using native assets still works (#1253). This would require 1. Migrating the Dart standalone embedder away from kernel embedding the native assets mapping, and 2. updating the native assets mapping after every hook execution.

Marking low prio as this is performance optimization and not required for v1.

@dcharkes dcharkes added P3 A lower priority bug or feature request package:hooks_runner labels Oct 10, 2024
auto-submit bot pushed a commit that referenced this issue Nov 28, 2024
This PR changes the caching behavior for hooks to be file content hashing instead of last modified timestamps.

Closes: #1593.

In addition to using file hashes, a timestamp is passed in to detect if files were modified during a build. The moment of hashing contents is after a build is finished, but we should consider files changed after the build started to invalidate the build. If this happens, the build succeeds, but the cache is invalidated and a warning is printed to the logger.

The implementation was modeled after the [`FileStore` in flutter_tools](https://github.com/flutter/flutter/blob/1e824af6bd217beb0cc201394c9832316b6150da/packages/flutter_tools/lib/src/build_system/file_store.dart). However, it was adapted to support caching directories and to match the code style in this repository.

Directory caching is defined as follows: the hash of the names of the children. This excludes recursive descendants, and excludes the contents of children. For recursive caching (and glob patterns), the populator of the cache should do the glob/recursion to add all directories and files.

### Testing

* The existing caching tests (such as `pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart`) cover caching behavior.
* Now that last-modified are no longer used, some sleeps have been removed from tests. � 

### Performance

Adding a stopwatch to pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart for the second invocation of the hooks so that it is cached. 

* `lastModified` timestamps: 0.028 seconds (pre this PR)
* `package:crypto` `md5`: 0.047 seconds (current PR)
* `package:xxh3` `xxh3`: 0.042 seconds

The implementation does not use parallel system IO for loading files (no `Future.wait`), but does use async I/O to allow flutter_tools to run other `Target`s in parallel.

The (pre and post this PR) implementation is fast enough for a handful of packages with native assets in a `flutter run`. The implementation (pre and post this PR) is not fast enough for hot restart / hot reload with 10+ packages with native assets. So, when exploring support for that, we'll need to revisit the implementation.

### Related issues not addressed in this PR

* #32
  * Changing environment variables should also cause the hook to rerun.
* #1578
  * Hook compilation can potentially be shared. (This requires taking more directory locks due to concurrent and re-entrant invocations.)
@dcharkes dcharkes added this to the Native Assets v1.x milestone Jan 7, 2025
@dcharkes dcharkes changed the title [assets] The asset builder system should compile hook/{build,link}.dart once. [native_assets_builder] The asset builder system should compile hook/{build,link}.dart once. Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 A lower priority bug or feature request package:hooks_runner
Projects
Status: No status
Development

No branches or pull requests

2 participants