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

Preserve specified AssetResolvers when performing a hot restart or updating the asset directory #21611

Merged
merged 12 commits into from
Oct 8, 2020
Merged

Preserve specified AssetResolvers when performing a hot restart or updating the asset directory #21611

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ff717e6
attempt sync throw from futurize
jonahwilliams Oct 4, 2020
bfa9783
attempt to preserve specified asset managers on reload and restart
jonahwilliams Oct 6, 2020
13fb0c1
revert unrelated changes
jonahwilliams Oct 6, 2020
6810d05
Update asset_manager.cc
jonahwilliams Oct 6, 2020
4bfc73a
Merge branch 'master' of github.com:flutter/engine into more_preserve
Oct 7, 2020
27b200a
hoist into shell
Oct 7, 2020
5a4d2ab
use magic iterator logic
Oct 7, 2020
980ecb7
actually compile
Oct 7, 2020
2e94df6
more doc comments, rename to IsValidAfterAssetManagerChange
Oct 8, 2020
5f920da
fix shell.cc
Oct 8, 2020
9a8269f
dont forget asset manager
Oct 8, 2020
c336ec7
woops
Oct 8, 2020
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
9 changes: 9 additions & 0 deletions assets/asset_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ void AssetManager::PushBack(std::unique_ptr<AssetResolver> resolver) {
resolvers_.push_back(std::move(resolver));
}

std::deque<std::unique_ptr<AssetResolver>> AssetManager::TakeResolvers() {
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 idea if this is reasonable :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that this is correct because resolvers_ is an lvalue here.

This is behaviorally the same as making a copy of the deque and clearing the original, but the compiler should be able to optimize this to not do the copy now.

@jason-simmons or @chinmaygarde could verify that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even with the copy, the most frequent number of elements seems to be 1, and with this change that will grow to 2

Copy link
Contributor

Choose a reason for hiding this comment

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

In case I wasn't clear, this way of doing things avoids a copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh ... good, right :)

return std::move(resolvers_);
}

// |AssetResolver|
std::unique_ptr<fml::Mapping> AssetManager::GetAsMapping(
const std::string& asset_name) const {
Expand All @@ -52,4 +56,9 @@ bool AssetManager::IsValid() const {
return resolvers_.size() > 0;
}

// |AssetResolver|
bool AssetManager::ShouldPreserve() const {
return false;
}

} // namespace flutter
5 changes: 5 additions & 0 deletions assets/asset_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,14 @@ class AssetManager final : public AssetResolver {

void PushBack(std::unique_ptr<AssetResolver> resolver);

std::deque<std::unique_ptr<AssetResolver>> TakeResolvers();

// |AssetResolver|
bool IsValid() const override;

// |AssetResolver|
bool ShouldPreserve() const override;

// |AssetResolver|
std::unique_ptr<fml::Mapping> GetAsMapping(
const std::string& asset_name) const override;
Expand Down
11 changes: 11 additions & 0 deletions assets/asset_resolver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,17 @@ class AssetResolver {

virtual bool IsValid() const = 0;

//----------------------------------------------------------------------------
/// @brief Certain asset resolvers need to be preserved when the asset
/// directory is updated or the isolate recreated. While some
/// could be recated from the settings object, the Android
/// specific asset resolvers require a reference to the JNI to
/// create.
///
/// @return Returns whether this resolver should be preserved.
///
virtual bool ShouldPreserve() const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we come up with a better name for this?

Maybe something like IsValidAfterIsolateRestart?

"Should" is a bit of a weak word to use (we usually prefer "must"), and it'd be helpful to have more context when reading the code about when or why I should care about this parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about something like:

Asset resolvers that are valid after a reload or a restart must be preserved and inserted into the newly created asset manager or run configuration. This allows the tooling to avoid copying assets through the device devFS on start, which has
reliability issues on iOS and is slow when the asset size is larger.

Copy link
Contributor

Choose a reason for hiding this comment

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

That might help with the docs, but I'm looking for a better member name than ShouldPreserve. Does IsValidAfterIsolateRestart?

And it this something we could combine with IsValid somehow if we parameterized it? Perhaps with a default value like IsValid(bool isolate_restarted = false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not just restarts though, its also the first hot reload. IsValidAfterAssetManagerChange maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. Just something that gives me a slightly better clue as I go to use the API and read the code. And then in the docs we can describe when an asset manager changes or why I'd want to check this.


[[nodiscard]] virtual std::unique_ptr<fml::Mapping> GetAsMapping(
const std::string& asset_name) const = 0;

Expand Down
9 changes: 8 additions & 1 deletion assets/directory_asset_bundle.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@

namespace flutter {

DirectoryAssetBundle::DirectoryAssetBundle(fml::UniqueFD descriptor)
DirectoryAssetBundle::DirectoryAssetBundle(fml::UniqueFD descriptor,
bool should_preserve)
: descriptor_(std::move(descriptor)) {
if (!fml::IsDirectory(descriptor_)) {
return;
}
should_preserve_ = should_preserve;
is_valid_ = true;
}

Expand All @@ -27,6 +29,11 @@ bool DirectoryAssetBundle::IsValid() const {
return is_valid_;
}

// |AssetResolver|
bool DirectoryAssetBundle::ShouldPreserve() const {
return should_preserve_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you missed 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.

Gotem

}

// |AssetResolver|
std::unique_ptr<fml::Mapping> DirectoryAssetBundle::GetAsMapping(
const std::string& asset_name) const {
Expand Down
6 changes: 5 additions & 1 deletion assets/directory_asset_bundle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,21 @@ namespace flutter {

class DirectoryAssetBundle : public AssetResolver {
public:
explicit DirectoryAssetBundle(fml::UniqueFD descriptor);
explicit DirectoryAssetBundle(fml::UniqueFD descriptor, bool should_preserve);
Copy link
Contributor

Choose a reason for hiding this comment

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

drop explicit since this is no longer a one arg ctor.


~DirectoryAssetBundle() override;

private:
const fml::UniqueFD descriptor_;
bool is_valid_ = false;
bool should_preserve_ = false;

// |AssetResolver|
bool IsValid() const override;

// |AssetResolver|
bool ShouldPreserve() const override;

// |AssetResolver|
std::unique_ptr<fml::Mapping> GetAsMapping(
const std::string& asset_name) const override;
Expand Down
4 changes: 4 additions & 0 deletions shell/common/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ void Engine::SetupDefaultFontManager() {
font_collection_.SetupDefaultFontManager();
}

std::shared_ptr<AssetManager> Engine::GetAssetManager() {
return asset_manager_;
}

bool Engine::UpdateAssetManager(
std::shared_ptr<AssetManager> new_asset_manager) {
if (asset_manager_ == new_asset_manager) {
Expand Down
3 changes: 3 additions & 0 deletions shell/common/engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,9 @@ class Engine final : public RuntimeDelegate,
// |RuntimeDelegate|
FontCollection& GetFontCollection() override;

// Return the asset manager associated with the current engine, or nullptr.
std::shared_ptr<AssetManager> GetAssetManager();

// |PointerDataDispatcher::Delegate|
void DoDispatchPacket(std::unique_ptr<PointerDataPacket> packet,
uint64_t trace_flow_id) override;
Expand Down
7 changes: 4 additions & 3 deletions shell/common/persistent_cache_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,10 @@ TEST_F(ShellTest, CanLoadSkSLsFromAsset) {
ResetAssetManager();
auto asset_manager = std::make_shared<AssetManager>();
RunConfiguration config(nullptr, asset_manager);
asset_manager->PushBack(
std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
asset_dir.path().c_str(), false, fml::FilePermission::kRead)));
asset_manager->PushBack(std::make_unique<DirectoryAssetBundle>(
fml::OpenDirectory(asset_dir.path().c_str(), false,
fml::FilePermission::kRead),
false));
CheckTwoSkSLsAreLoaded();

// 3rd, test the content of the SkSLs in the asset.
Expand Down
9 changes: 5 additions & 4 deletions shell/common/run_configuration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ RunConfiguration RunConfiguration::InferFromSettings(

if (fml::UniqueFD::traits_type::IsValid(settings.assets_dir)) {
asset_manager->PushBack(std::make_unique<DirectoryAssetBundle>(
fml::Duplicate(settings.assets_dir)));
fml::Duplicate(settings.assets_dir), true));
}

asset_manager->PushBack(
std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
settings.assets_path.c_str(), false, fml::FilePermission::kRead)));
asset_manager->PushBack(std::make_unique<DirectoryAssetBundle>(
fml::OpenDirectory(settings.assets_path.c_str(), false,
fml::FilePermission::kRead),
true));

return {IsolateConfiguration::InferFromSettings(settings, asset_manager,
io_worker),
Expand Down
44 changes: 26 additions & 18 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,10 +1401,20 @@ bool Shell::OnServiceProtocolRunInView(
configuration.SetEntrypointAndLibrary(engine_->GetLastEntrypoint(),
engine_->GetLastEntrypointLibrary());

configuration.AddAssetResolver(
std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
asset_directory_path.c_str(), false, fml::FilePermission::kRead)));
configuration.AddAssetResolver(RestoreOriginalAssetResolver());
configuration.AddAssetResolver(std::make_unique<DirectoryAssetBundle>(
fml::OpenDirectory(asset_directory_path.c_str(), false,
fml::FilePermission::kRead),
false));

// Preserve any original asset resolvers to avoid syncing unchanged assets
// over the DevFS connection.
auto old_resolvers = engine_->GetAssetManager()->TakeResolvers();
for (uint64_t i = 0; i < old_resolvers.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

use something like for (auto& old_resolver : engine_->GetAssetManager()->TakeResolvers())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to handle asset manager being a nullptr too, right?

Copy link
Member

Choose a reason for hiding this comment

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

yes - looking at the Engine it appears that the asset manager could be null if something went wrong during initialization or a restart

auto old_resolver = std::move(old_resolvers[i]);
if (old_resolver->ShouldPreserve()) {
configuration.AddAssetResolver(std::move(old_resolver));
}
}

auto& allocator = response->GetAllocator();
response->SetObject();
Expand Down Expand Up @@ -1517,10 +1527,20 @@ bool Shell::OnServiceProtocolSetAssetBundlePath(

auto asset_manager = std::make_shared<AssetManager>();

asset_manager->PushFront(RestoreOriginalAssetResolver());
asset_manager->PushFront(std::make_unique<DirectoryAssetBundle>(
fml::OpenDirectory(params.at("assetDirectory").data(), false,
fml::FilePermission::kRead)));
fml::FilePermission::kRead),
false));

// Preserve any original asset resolvers to avoid syncing unchanged assets
// over the DevFS connection.
auto old_resolvers = engine_->GetAssetManager()->TakeResolvers();
for (uint64_t i = 0; i < old_resolvers.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

same here

auto old_resolver = std::move(old_resolvers[i]);
if (old_resolver->ShouldPreserve()) {
asset_manager->PushBack(std::move(old_resolver));
}
}

if (engine_->UpdateAssetManager(std::move(asset_manager))) {
response->AddMember("type", "Success", allocator);
Expand Down Expand Up @@ -1624,16 +1644,4 @@ void Shell::OnDisplayUpdates(DisplayUpdateType update_type,
display_manager_->HandleDisplayUpdates(update_type, displays);
}

// Add the original asset directory to the resolvers so that unmodified assets
// bundled with the application specific format (APK, IPA) can be used without
// syncing to the Dart devFS.
std::unique_ptr<DirectoryAssetBundle> Shell::RestoreOriginalAssetResolver() {
if (fml::UniqueFD::traits_type::IsValid(settings_.assets_dir)) {
return std::make_unique<DirectoryAssetBundle>(
fml::Duplicate(settings_.assets_dir));
}
return std::make_unique<DirectoryAssetBundle>(fml::OpenDirectory(
settings_.assets_path.c_str(), false, fml::FilePermission::kRead));
};

} // namespace flutter
4 changes: 4 additions & 0 deletions shell/platform/android/apk_asset_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ bool APKAssetProvider::IsValid() const {
return true;
}

bool APKAssetProvider::ShouldPreserve() const {
return true;
}

class APKAssetMapping : public fml::Mapping {
public:
APKAssetMapping(AAsset* asset) : asset_(asset) {}
Expand Down
3 changes: 3 additions & 0 deletions shell/platform/android/apk_asset_provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ class APKAssetProvider final : public AssetResolver {
// |flutter::AssetResolver|
bool IsValid() const override;

// |flutter::AssetResolver|
bool ShouldPreserve() const override;

// |flutter::AssetResolver|
std::unique_ptr<fml::Mapping> GetAsMapping(
const std::string& asset_name) const override;
Expand Down
9 changes: 5 additions & 4 deletions shell/testing/tester_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,11 @@ int RunTester(const flutter::Settings& settings,

auto asset_manager = std::make_shared<flutter::AssetManager>();
asset_manager->PushBack(std::make_unique<flutter::DirectoryAssetBundle>(
fml::Duplicate(settings.assets_dir)));
asset_manager->PushBack(
std::make_unique<flutter::DirectoryAssetBundle>(fml::OpenDirectory(
settings.assets_path.c_str(), false, fml::FilePermission::kRead)));
fml::Duplicate(settings.assets_dir), true));
asset_manager->PushBack(std::make_unique<flutter::DirectoryAssetBundle>(
fml::OpenDirectory(settings.assets_path.c_str(), false,
fml::FilePermission::kRead),
true));

RunConfiguration run_configuration(std::move(isolate_configuration),
std::move(asset_manager));
Expand Down