Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Jul 6, 2022

fix flutter/flutter#107091

On the Android platform, the current implementation is that multiple lightweight engines share the same AssetManager, and the relationship between the engine and the AssetManager is Nv1. After engine restart, each engine will regenerate its own AssetManager, and the relationship between the engine and the AssetManager becomes 1v1.

Due to the following code, only one of these new AssetManagers will hold the original APKAssetProvider.
https://github.com/flutter/engine/blob/main/shell/common/shell.cc#L1632-L1641

  // Preserve any original asset resolvers to avoid syncing unchanged assets
  // over the DevFS connection.
  auto old_asset_manager = engine_->GetAssetManager();
  if (old_asset_manager != nullptr) {
    for (auto& old_resolver : old_asset_manager->TakeResolvers()) {
      if (old_resolver->IsValidAfterAssetManagerChange()) {
        configuration.AddAssetResolver(std::move(old_resolver));
      }
    }
  }

proposed solution

  1. Lightweight engines no longer share AssetManager, each engine has own AssetManager, and each AssetManager has an APKAssetProvider inside.

  2. There is an APKAssetProviderImpl inside the APKAssetProvider, which is used to encapsulate the specific implementation. In a lightweight engine scenario, multiple APKAssetProviders share the same APKAssetProviderImpl

  3. APKAssetProvider provides a Clone method to create a new APKAssetProvider, the new provider shares APKAssetProviderImpl with the current provider.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

@ColdPaleLight ColdPaleLight marked this pull request as draft July 6, 2022 13:24
@ColdPaleLight
Copy link
Member Author

(Unittest added)

@ColdPaleLight ColdPaleLight marked this pull request as ready for review July 7, 2022 07:15
@dnfield
Copy link
Contributor

dnfield commented Jul 7, 2022

This seems reasonable to me, tagging Jason and Gary to take a look as well.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 8, 2022
@auto-submit auto-submit bot merged commit e6b17e0 into flutter:main Jul 8, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2022
@dnfield
Copy link
Contributor

dnfield commented Feb 16, 2024

This has an unfortunate side effect: lightweight engines now end up reloading all the same fonts because the test here fails:

if (asset_manager_ == new_asset_manager) {

This means lightweight engines end up using significantly more memory for some applications.

/cc @jiahaog

@xster
Copy link
Member

xster commented Feb 16, 2024

@jiahaog this might be what you're seeing in your memory tests? The results did look unexpected to me.

jiahaog added a commit to jiahaog/flutter-engine that referenced this pull request Feb 21, 2024
…after engines restart (flutter#34473)"

This reverts commit e6b17e0.

Manually resolved the following conflicts:

- ci/licenses_golden/licenses_flutter
- shell/platform/android/apk_asset_provider.cc
- shell/platform/android/apk_asset_provider_unittests.cc
jiahaog added a commit to jiahaog/flutter-engine that referenced this pull request Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-android

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[add-to-app] multi-engine loading assets throws exception when loading assets after hot restarting when using flutter attach

4 participants