Add support for loading asset directly from ImmutableBuffer #32999

jonahwilliams · 2022-04-29T00:48:20Z

Removes multiple copies from asset loading by directly instantiating an SkData from a mapping in the engine. This about doubles asset loading speed on a release windows desktop build. Need to test on a lower end device too. Avoids round-triping bytes through the Dart heap which should also help with GC pressure. Only useful for Image APIs though, anywhere the framework ends up reading the bytes requires a Uint8List.

This will require substantial changes to the ImageProvider APIs to take advantage of this change

Based off of previous attempt #21153

jonahwilliams · 2022-04-29T00:48:34Z

lib/ui/painting.dart

+///
+/// The returned future can complete with an error if the image decoding has
+/// failed.
+Future<Codec> instantiateImageCodecFromBuffer(


Couldn't think of what to do besides copy the API

jonahwilliams · 2022-04-29T00:49:25Z

lib/ui/painting.dart

+  static ImmutableBuffer? fromAsset(String assetKey) {
+    final ImmutableBuffer buffer = ImmutableBuffer._(0);
+    bool success = false;
+    buffer._initFromAsset(assetKey, (void result) {


As far as I can tell, neither this method nor fromUint8List are in any way async, and instead they fully perform all synchronous work on the event loop they are called from and only delay returning. I don't see the point in doing that

I'm not sure this works.

Today asset loading happens to run on the UI thread, but I'm hesitant to write this API in such a way that boxes us into doing that or breaking people.

Because of the length field, the data is not completely opaque - so we can't really even do a trick here and say that we'll return eagerly and join a thread later when we actually need the data, because the caller might immediately query the lenght.

You're right that the Uint8List one could have been sync - I think it was made async because the idea was someday we'd have things that needed to be async, and it'd be better if callsites using this already were prepared for async code than not.

makes sense!

dnfield · 2022-04-29T23:05:01Z

lib/ui/painting.dart

+  static ImmutableBuffer? fromAsset(String assetKey) {
+    final ImmutableBuffer buffer = ImmutableBuffer._(0);
+    bool success = false;
+    buffer._initFromAsset(assetKey, (void result) {


I'm not sure this works.

Today asset loading happens to run on the UI thread, but I'm hesitant to write this API in such a way that boxes us into doing that or breaking people.

Because of the length field, the data is not completely opaque - so we can't really even do a trick here and say that we'll return eagerly and join a thread later when we actually need the data, because the caller might immediately query the lenght.

dnfield · 2022-04-29T23:08:58Z

lib/ui/painting.dart

+  static ImmutableBuffer? fromAsset(String assetKey) {
+    final ImmutableBuffer buffer = ImmutableBuffer._(0);
+    bool success = false;
+    buffer._initFromAsset(assetKey, (void result) {


You're right that the Uint8List one could have been sync - I think it was made async because the idea was someday we'd have things that needed to be async, and it'd be better if callsites using this already were prepared for async code than not.

dnfield · 2022-04-29T23:10:39Z

lib/ui/painting.dart

+      success = true;
+    });
+    if (!success) {
+      return null;


I think this should throw rather than returning null.

For example, if you try to open an asset or file that doesn't exist that should throw. In other cases like this API we do not ignore the error message sent from native.

dnfield · 2022-04-29T23:10:51Z

testing/dart/assets_test.dart

+
+void main() {
+  test('Loading an asset that does not exist returns null', () {
+    expect(ui.ImmutableBuffer.fromAsset('ThisDoesNotExist'), null);


Same comment as above - I think this should throw.

dnfield · 2022-04-29T23:11:01Z

testing/dart/assets_test.dart

+  });
+
+  test('returns the bytes of a bundled asset', () {
+    expect(ui.ImmutableBuffer.fromAsset('assets/DashInNooglerHat.jpg'), isNotNull);


consider asserting the length is right.

let me check what it actually is first...

dnfield · 2022-04-29T23:11:34Z

@chinmaygarde FYI - when this was added we talked about use cases like this, I think.

jonahwilliams · 2022-04-29T23:44:56Z

Yeah, we can definitely make it async to give us more room in the future.

jonahwilliams · 2022-04-29T23:51:52Z

lib/ui/painting.dart

+  static Future<ImmutableBuffer> fromAsset(String assetKey) {
+    final ImmutableBuffer instance = ImmutableBuffer._(0);
+    return _futurize((_Callback<void> callback) {
+      return instance._initFromAsset(assetKey, callback);


_futurize handles throwing an exception if this method returns a non-null string.

jonahwilliams · 2022-04-29T23:53:13Z

updated to make the process async

dnfield

This LGTM but I'd appreciate a review from Chinmay too.

…to immutable_buffer

jonahwilliams · 2022-04-30T04:12:39Z

lib/ui/painting/immutable_buffer.cc

+  const void* bytes = static_cast<const void*>(data->GetMapping());
+  auto size = data->GetSize();
+  void* peer = reinterpret_cast<void*>(data.release());
+  SkData::ReleaseProc proc = [](const void* ptr, void* context) {};


I think for this to work correctly it needs to release the mapping somehow

dnfield · 2022-05-02T16:01:52Z

fml/mapping.cc

+MappingReleaseProc FileMapping::GetReleaseProc() {
+  MappingReleaseProc proc = [](const void* ptr, void* context) {
+    auto* mapping = static_cast<fml::FileMapping*>(context);
+    mapping->~FileMapping();
+  };
+  return proc;
+}
+


I believe the idiom for this is ot use a NonOwnedMapping.

Directly calling a destructor is pretty weird and should be avoided.

In particular, NonOwnedMapping can be handed a release proc to run, and for this case we'd give it the SkData release proc.

I think that is backwards of what we need though - non owned mapping calls the release proc given in its destructor but the problem is that with the release required to pass the mapping to skdata, the destructor won't get called - right?

jonahwilliams · 2022-05-02T18:03:53Z

Updated to just use delete

chinmaygarde

Other than the one question about thread safety of the asset manager mappings, lgtm.

chinmaygarde · 2022-05-02T18:17:56Z

lib/ui/painting/immutable_buffer.cc

+                                                    ->platform_configuration()
+                                                    ->client()
+                                                    ->GetAssetManager();
+  std::unique_ptr<fml::Mapping> data = asset_manager->GetAsMapping(asset_name);


Are these mappings thread safe? These usually come from flutter::AssetResolvers and looking at the inheritance diagram from these, flutter::APKAssetProvider seems to be a subclass. Its assets are backed by AAsset handles whose thread safety notes say "AAsset objects are NOT thread-safe, and should not be shared across threads.". Since there used to be a copy earlier, it sort of didn't matter. But now that these are referenced across thread (for texture decompression), there may be threading violations that we have perhaps not contended with.

Hm. Sounds like we can't avoid the copy in that case.

If we didn't expose the lenght, we could get away with this being something that's done entirely on the decoder thread though right? We could just give that a way to acquire the data and then let it do the whole thing there.

We don't need the length in the framework for this case. We could always say "length might be -1 for certain instances not created from a uint8list"

we could also copy once into an skdata - that would still reduce the number of copies by 1. Though having a single createCodecFromAsset method would mean we could do the entire thing on the right thread, i'm not sure how well that works with the framework wanting to abstract byte acquisition from image decoding.

I don't think adding a special case for asset images would be a bad thing fwiw, but it would be a special case.

jonahwilliams · 2022-05-06T16:43:17Z

Looked at this some more:

We'd have to move the load method all the way back to the Codec decode method in order to avoid needing to hold an SkData. I'm a bit concerned that this won't really fit into the image provider/asset bundle logic we support today. I think next week I'll compare the performance of ImmutableBuffer.fromAsset with a single copy to see what the cost is

jonahwilliams · 2022-05-10T21:44:40Z

ImmutableBuffer.fromAsset on android w/ copy: 28, 26, 26, 26, 21, 28
rootBundle.load + ImmutableBuffer.fromUint8List: 46, 46, 41, 43, 46,

So even with a copy, we still improve asset load time. And on non-android platforms, its even better

jonahwilliams · 2022-05-10T23:01:54Z

Also: this avoids round-tripping the bytes through the dart heap, which is beneficial from a GC perspective. Potentially more so than avoiding a copy.

dnfield · 2022-05-10T23:46:45Z

lib/ui/painting/immutable_buffer.cc

+#if FML_OS_ANDROID
+  // fml::Mapping backed by android assets are not thread safe.
+  auto sk_data = MakeSkDataWithCopy(bytes, size);
+#else
+  SkData::ReleaseProc proc = [](const void* ptr, void* context) {
+    delete static_cast<fml::Mapping*>(context);
+  };
+  void* peer = reinterpret_cast<void*>(data.release());
+  auto sk_data = SkData::MakeWithProc(bytes, size, proc, peer);
+#endif  // FML_OS_ANDROID


I don't think we can assume this is thread safe on any platform - we should just default to making copies everywhere, since the fml::Mapping interface doesn't declare itself as thread safe.

dnfield · 2022-05-11T00:13:53Z

lib/ui/painting/immutable_buffer.cc

+#if FML_OS_ANDROID
+  auto sk_data = MakeSkDataWithCopy(bytes, size);
+#else
+  auto sk_data = SkData::MakeWithCopy(bytes, size);
+#endif  // FML_OS_ANDROID
+


nit: I think you can just call MakeSkDataWithCopy since it's already ifdefed like this below anyway.

oh woops, misread that - thanks!

dnfield

LGTM with nit. Would appreciate LGTM from @chinmaygarde as well.

jonahwilliams · 2022-05-11T02:36:40Z

Since this requires a framework change to utilize, i'll land this before I add a benchmark.

…lutter/engine#32999)

Add support for loading asset directly from ImmutableBuffer

9623154

flutter-dashboard bot added the platform-web label Apr 29, 2022

jonahwilliams commented Apr 29, 2022

View reviewed changes

jonahwilliams requested a review from dnfield April 29, 2022 00:49

jonahwilliams mentioned this pull request Apr 29, 2022

Utilize ImmutableBuffer.fromAsset for faster image loading flutter/flutter#102797

Closed

jonahwilliams added 2 commits April 28, 2022 17:58

Update platform_configuration.h

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

ba639ba

make delegation the same

fe93694

jonahwilliams mentioned this pull request Apr 29, 2022

Don't hardcode ink sparkle SPIR-V flutter/flutter#102674

Merged

jonahwilliams added 4 commits April 29, 2022 12:59

update gn file

7726151

use fixture

9f45888

add asset dir

6963e65

Merge branch 'master' of github.com:flutter/engine into immutable_buffer

5e2f694

dnfield reviewed Apr 29, 2022

View reviewed changes

dnfield requested a review from chinmaygarde April 29, 2022 23:11

make async

769a9d8

jonahwilliams commented Apr 29, 2022

View reviewed changes

Update painting.dart

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

68f5b9b

jonahwilliams requested a review from dnfield April 29, 2022 23:53

dnfield reviewed Apr 29, 2022

View reviewed changes

jonahwilliams added 5 commits April 29, 2022 17:17

Actually set length

f216c48

Merge branch 'immutable_buffer' of github.com:jonahwilliams/engine in…

bae7201

…to immutable_buffer

Update painting.dart

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

61ec3a3

Update painting.dart

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

614a997

Update assets_test.dart

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

6bb5767

jonahwilliams commented Apr 30, 2022

View reviewed changes

something like this

8d31015

dnfield reviewed May 2, 2022

View reviewed changes

use delete

512a2b6

jonahwilliams added affects: framework affects: engine labels May 2, 2022

chinmaygarde reviewed May 2, 2022

View reviewed changes

jonahwilliams added 2 commits May 10, 2022 14:45

special android copy

d27b4eb

++

75b86f9

jonahwilliams requested a review from dnfield May 10, 2022 21:48

jonahwilliams requested a review from chinmaygarde May 10, 2022 23:06

dnfield reviewed May 10, 2022

View reviewed changes

jonahwilliams added 2 commits May 10, 2022 17:00

always copy

7082bdb

Merge branch 'master' of github.com:flutter/engine into immutable_buffer

3c8a0e3

dnfield reviewed May 11, 2022

View reviewed changes

dnfield approved these changes May 11, 2022

View reviewed changes

chinmaygarde approved these changes May 11, 2022

View reviewed changes

Update immutable_buffer.cc

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

66ccfc5

jonahwilliams added the waiting for tree to go green label May 11, 2022

jonahwilliams mentioned this pull request May 11, 2022

Consider implementing ImmutableBuffer.fromAsset on the web flutter/flutter#103454

Open

fluttergithubbot merged commit 70c5bdf into flutter:main May 11, 2022

jonahwilliams mentioned this pull request May 11, 2022

add image load benchmark flutter/flutter#103459

Merged

jonahwilliams deleted the immutable_buffer branch May 11, 2022 03:47

engine-flutter-autoroll mentioned this pull request May 11, 2022

Roll Flutter Engine from 6291508027f9 to 70c5bdfa3bfd (1 revision) flutter/flutter#103460

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 11, 2022

70c5bdf Add support for loading asset directly from ImmutableBuffer (f…

b6088d6

…lutter/engine#32999)

a-gms mentioned this pull request Jul 12, 2022

Loading asset images causes animation jank flutter/flutter#107487

Closed

renancaraujo mentioned this pull request Sep 9, 2022

Asset bundle fails to load assets on tests in specific cases after 3.3.0 flutter/flutter#111285

Closed

Add support for loading asset directly from ImmutableBuffer #32999

Add support for loading asset directly from ImmutableBuffer #32999

Conversation

jonahwilliams commented Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Apr 29, 2022

jonahwilliams commented Apr 29, 2022

Choose a reason for hiding this comment

jonahwilliams commented Apr 29, 2022

dnfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented May 2, 2022

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented May 6, 2022

jonahwilliams commented May 10, 2022

jonahwilliams commented May 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

jonahwilliams commented May 11, 2022

jonahwilliams commented Apr 29, 2022 •

edited

Loading