Skip to content
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

[native_assets_cli][native_assets_builder] Hot reload and hot restart #1207

Open
dcharkes opened this issue Jun 13, 2024 · 8 comments
Open
Labels
P2 A bug or feature request we're likely to work on package:native_assets_builder package:native_assets_cli

Comments

@dcharkes
Copy link
Collaborator

Whether hot reload and hot restart can be supported depends on the asset type.

  • For bundled native code, I expect us to be able to support hot restart for dynamic libraries. (Static linking & static libraries of course requires rebuilding & restarting the native app.) [native_assets] Hot restart native libs sdk#55850
  • For bundled data assets that are used as bytes/strings in Dart code, we should be able to both support hot restart and hot reload.
  • For other asset types it depends on the embedder if that embedder knows how to hot reload / hot restart such asset types. (Jars for example possibly if we implement such thing in Flutter.)

Now this poses some questions for how the protocol should work:

If we blindly invoke the build and link hooks, we might get longer running builds for asset types that are not going to be hot-reload. E.g. hot reloading data assets should be fast especially if these assets are on disk already, but a locally built dynamic library will take considerably longer. We could add a BuildConfig / LinkConfig option with filteredAssetTypes.

However, adding such configuration option would break caching between the full build and the for-hot-reload build. (For example the list of dependencies is combined for all assets.)

We could fix caching by splitting the initial invocation of build and link hooks into multiple invocations per asset type. Then rerunning for a specific asset type would work.

cc @mosuem @HosseinYousefi @liamappelbe @mkustermann

@dcharkes dcharkes added P2 A bug or feature request we're likely to work on package:native_assets_cli package:native_assets_builder labels Jun 13, 2024
@mosuem
Copy link
Member

mosuem commented Jun 13, 2024

For bundled data assets that are used as bytes/strings in Dart code, we should be able to both support hot restart and hot reload.

Does is make sense to rebuild assets on hot reload? Some assets might take a long time to build or link... Should the user specify if an asset is fast-reloadable? It is impossible to tell for the hooks if an asset takes long to build.

@mkustermann
Copy link
Member

If we blindly invoke the build and link hooks, we might get longer running builds for asset types that are not going to be hot-reload. E.g. hot reloading data assets should be fast especially if these assets are on disk already, but a locally built dynamic library will take considerably longer. We could add a BuildConfig / LinkConfig option with filteredAssetTypes.

I don't think this is an issue, because the building tool specifies which asset types are supported / should be built. So in

  • the initial build the invocation of hook/build.dart, the build config will say
    => buildConfig.assetTypes: ['native-library', 'fragment-shader', 'image', ...]
  • in the hot-restart (possibly hot-reload) invocation of hook/build.dart the build config will say
    => buildConfig.assetTypes: ['fragment-shader', 'image', ...] (i.e. a subset)

So the flutter run can decide which asset types it may support for hot-reload and just ask for those, no?

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jun 13, 2024

So the flutter run can decide which asset types it may support for hot-reload and just ask for those, no?

I believe the caching would not work in that case. The cached results of fragment-shader and image from the main build would not be reused in the first hot reload if none of the assets actually changed.

(Edit: Maybe that is not an issue. Flutter also prebuilds a kernel file as a copy... So maybe we can pre-populate the hot-reload assets as well.)

@mkustermann
Copy link
Member

I believe the caching would not work in that case. The cached results of fragment-shader and image from the main build would not be reused in the first hot reload if none of the assets actually changed.

Please clearify what you mean here. If we run flutter run and then later on run flutter run (or flutter build) again, then it should perform incremental builds. Why is this any different in hot-reload? Is the implementation currently setup to not support this, or is this a fundamental issue?

@mkustermann
Copy link
Member

As a side note: We should also ensure that dependencies (e.g. .d files) are recorded for each asset and not on a global level. This will ensure that if the building tool only wants to rebuild certain assets (e.g. shaders) only changes to those assets will cause an invocation of hook/build.dart

@dcharkes
Copy link
Collaborator Author

dcharkes commented Jun 13, 2024

Why is this any different in hot-reload? Is the implementation currently setup to not support this, or is this a fundamental issue?

This is a conceptual issue: We report the dependencies of all assets together. If only a subset of assets is required for hot reload, we can't use the dependencies list for the full build to determine whether or not we need to re-invoke the hook or not. So the first build hook invocation on the first hot reload will always need to run (unless none of the dependencies changed, also of the assets that cannot be hot reloaded).

As a side note: We should also ensure that dependencies (e.g. .d files) are recorded for each asset and not on a global level. This will ensure that if the building tool only wants to rebuild certain assets (e.g. shaders) only changes to those assets will cause an invocation of hook/build.dart

This, exactly.

@dcharkes
Copy link
Collaborator Author

An alternative design suggested by @HosseinYousefi: Center the hook interaction around whether something is a hot-restart invocation or not.

  • BuildConfig gets a bool isHotRestart getter.
  • BuildConfig gets a Iterable<String> supportHotreloadAssetTypes getter.
  • BuildOutput gets a addDependencies(Iterable<Uri> files, bool invokeForHotReload).

In the style of #1375 option 1, we would run hot-restart hook invocations in the same directory as the cold start invocation. Hook writers can then simply re-output all assets that they don't want to rebuild and rebuild the assets that they want. This enables hook writers to decide per asset if they want it to be hot restarted (maybe some heavy to compile data asset should not be hot restarted).

The caching in the native assets builder needs to slightly be changed:

  • For hot restarts, we can look at the timestamp of the last hook invocation compared to the last-modified on all dependencies.
  • For cold starts, we need to either have saved the timestamp of the last cold start hook invocation, or we can check all the timestamps of all asset files produced.

@mosuem This is a solution for what we were talking about last week on whether the hot-restart build directory is shared with the cold-start build directory. WDYT?

The main idea of this approach is to keep the concepts simpler:

  • No need for dependencies per asset or per asset type. Hook writers explicitly say whether deps are for hot-restart or not
  • No need for dependencies/assetIds that need to be changed in the build config. Hook writers just use the file system for caching (option 1 in [native_assets_cli] In-hook caching #1375).
  • Single directory for where all builds are happening (for the same OS and same architecture), and users can rely on state.
    • And in addition this helps caching, the first hot-restart doesn't have to redo work that was done for the first cold start.

@dcharkes
Copy link
Collaborator Author

Whether we have dependencies per asset (#1368) is connected to whether our protocol has a concept of dry run. (Thanks @mkustermann!)

  • If we have a dryRun (possibly to be renamed to listAssets),
    • The non-dry-run build invocation should not be able to add, remove or rename assets.
    • We would need hookDependencies in the dry run which influences which assets are reported.
    • We would have assetDependencies in the build hook which report dependencies per asset.
    • The build config should list which assets should be built.
    • If we want to support adding assets in hot restart, we need to rerun the dryRun to see if new assets are listed, and then also rerun the non dry run to actually produce those assets.
  • If we remove dryRun
    • Then the build hook outputs the list of assets.
    • The dependencies of the build hook contain both the dependencies that influence the list of assets as well as the assets themselves.
    • The build hook will be rerun for hot restarts.
    • It is natural that hot restarts can add, remove and rename assets.

Getting rid of dry run would simplify the abstractions for our users.

Getting rid of dry run requires:

  • Implementing file locking based concurrency control between flutter run (needs the asset list) and flutter assemble (needs the asset list). [native_assets_builder] Concurrent execution of NativeAssetsBuilder #1319
    • Addressing that issue should in general make the flutter build faster, the first one of the two should lock, and the other should wait until the lock is released. (per arch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on package:native_assets_builder package:native_assets_cli
Projects
None yet
Development

No branches or pull requests

3 participants