Eliminates Dart->Host copy for mutable responses #34018
Conversation
gaaclarke
force-pushed
the
eliminate-copy-response
branch
from
aac6765 to
a236ed5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I'm not sure if this case requires a test exemption since it has a test but it exists in the other repo. |
| /*length=*/data_size, | ||
| /*peer=*/data.release(), | ||
| /*external_allocation_size=*/data_size, | ||
| /*callback=*/MappingFinalizer); |
There was a problem hiding this comment.
Positive change. But, the previous version of the response was a copy that the Dart code could potentially modify. Now, modifications will be potentially fatal (if possible at all when using an external typed data, I'm not sure).
I suspect not too many use cases modify the response buffer, but I'm not sure. Does this require a migration?
There was a problem hiding this comment.
cc @dnfield @jonahwilliams who might know more about the mutability of these buffers.
There was a problem hiding this comment.
I think we had looked into this earlier and concluded we couldn't use this API because we currently expose this buffers as mutable.
If we exposed them as immutable (besides being a breaking change), Dart will choke on them if they end up getting fed back into an engine API: dart-lang/sdk#42785
There was a problem hiding this comment.
We need some kind of support from dart for this use case - immutable data that doesn't incur runtime penalties related to using UnmodifiableTypedData. /cc @lhrn - I think we've talked about this before
There was a problem hiding this comment.
Why is it a problem if they are mutated? Since we've migrated the data buffers to unique pointers we can guarantee that Dart is the only one that has access to the data.
There was a problem hiding this comment.
Maybe that's the situation you are talking about @chinmaygarde? What if we expose a FileMapping to read-only memory and Dart tries to modify the data? We have tests that are sending and receiving platform channel messages. If we were modifying the buffers in our code, now or in the future, those tests would fail. Often times that data is just used to read to deserialize the data being passed and another copy is made at deserialization. The user never sees it.
If someone is using a BinaryCodec they may get access to a databuffer that will crash if they try to write to it (I'm not sure if we've eliminated all copies now). It will pretty obvious when that is attempted though. That isn't a breaking change unless it breaks any tests. If a user does run into this problem the solution would be pretty obvious, they need to copy the data.
There was a problem hiding this comment.
I was able to run 'customer: money' with no problems with this change, but framework tests do fail:
aaclarke-macbookpro2:flutter aaclarke$ flutter --local-engine-src-path ~/dev/engine/src --local-engine=host_debug_unopt test
05:12 +6331 ~15: /Users/aaclarke/dev/flutter/packages/flutter/test/material/tab_bar_theme_test.dart: Tab bar theme - beveled rect indicator
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following _Exception was thrown while running async test code:
Exception: Invalid image data
When the exception was thrown, this was the stack:
#0 _futurize (dart:ui/painting.dart:5779:5)
#1 ImageDescriptor.encoded (dart:ui/painting.dart:5634:12)
#2 instantiateImageCodecFromBuffer (dart:ui/painting.dart:2098:60)
#3 instantiateImageCodec (dart:ui/painting.dart:2056:10)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)
════════════════════════════════════════════════════════════════════════════════════════════════════
05:12 +6331 ~15 -1: /Users/aaclarke/dev/flutter/packages/flutter/test/material/tab_bar_theme_test.dart: Tab bar theme - beveled rect indicator [E]
Test failed. See exception logs above.
The test description was: Tab bar theme - beveled rect indicator
I'll see if I can address this, dropping back into a Draft. Worst case scenario we can selectively perform a copy for assets. This copy is not required for platform channels.
There was a problem hiding this comment.
(I can't reproduce this failure again now, investigating to see if it is non-deterministic)
There was a problem hiding this comment.
I'm unable to prove if that failure is actually a bug. It's hard when I can't reproduce it. If we are supplying a mmaped file to dart where previously we were supplying a copy, editing the file on disk could create a problem where the ui thread would encounter a file that changed on disk underneath it. It may be purely academic if in practice we are always passing mmaped files that are never modified on disk. Also, that wouldn't be a new bug if the modifier is not the ui thread (this change just makes it more likely).
There was a problem hiding this comment.
Why is it a problem if they are mutated? Since we've migrated the data buffers to unique pointers we can guarantee that Dart is the only one that has access to the data.
I was actually thinking about what happens if pages marked readonly are written to by Dart. That would be a segmentation fault. But you're also right about flutter::APKAssetMappings which are backed by AAssets that are not thread safe.
I am positive this is a contrived use case but it ought to be easy to write a test for this in the shell_unittests harness. I am actually curious if we can cause a segfault this way.
|
For asset loading the time change for a 1MB payload is: 543.29 µs to 339.95 µs (37%) Benchmark: flutter/flutter#105982 |
|
I'm totally convinced that not doing the copy is faster and also saves memory. But I'm not sure if this is going to cause segfaults in cases where a PROT_READ only mmaped buffer response is written to from Dart and also doesn't cause threading violations in cases APKAssetMappings. I was looking into how to verify that this is safe and the guidance to reproduce the contrived use-case would be to duplicate |
|
I had a chat with @jonahwilliams and @dnfield. We decided:
|
|
Hey, can't the operating system do copy-on-write with mmap? https://pythonspeed.com/articles/reduce-memory-array-copies/ This would be something to investigate. |
Looks like we are already doing copy-on-write (https://github.com/gaaclarke/engine/blob/a236ed5db78c665eb66de540a4fcda57c7862c71/fml/platform/posix/mapping_posix.cc#L75:L75), so this probably isn't a crasher. I'll have to write the test that Chinmay is suggesting tomorrow. Potentially we can remove all copies except apk assets which are not threadsafe. |
|
I was able to verify that copy-on-write works, and it's not what we are currently doing. I had to apply the following patch: diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc
index 7896756614..c0d0e0d844 100644
--- a/assets/directory_asset_bundle.cc
+++ b/assets/directory_asset_bundle.cc
@@ -50,8 +50,12 @@ std::unique_ptr<fml::Mapping> DirectoryAssetBundle::GetAsMapping(
return nullptr;
}
- auto mapping = std::make_unique<fml::FileMapping>(fml::OpenFile(
- descriptor_, asset_name.c_str(), false, fml::FilePermission::kRead));
+ auto mapping = std::make_unique<fml::FileMapping>(
+ fml::OpenFile(descriptor_, asset_name.c_str(), false,
+ fml::FilePermission::kRead),
+ std::initializer_list<fml::FileMapping::Protection>(
+ {fml::FileMapping::Protection::kRead,
+ fml::FileMapping::Protection::kWrite}));
if (!mapping->IsValid()) {
return nullptr;
diff --git a/fml/platform/posix/mapping_posix.cc b/fml/platform/posix/mapping_posix.cc
index aff3d645d3..52c878e4f7 100644
--- a/fml/platform/posix/mapping_posix.cc
+++ b/fml/platform/posix/mapping_posix.cc
@@ -72,7 +72,7 @@ FileMapping::FileMapping(const fml::UniqueFD& handle,
auto* mapping =
::mmap(nullptr, stat_buffer.st_size, ToPosixProtectionFlags(protection),
- is_writable ? MAP_SHARED : MAP_PRIVATE, handle.get(), 0);
+ MAP_PRIVATE, handle.get(), 0);
if (mapping == MAP_FAILED) {
return;I propose we:
That way we can pass ownership of the read-write memory to Dart and a memcpy will only happen when one is required. |
gaaclarke
force-pushed
the
eliminate-copy-response
branch
from
a236ed5 to
2b16e4a
Compare
gaaclarke
force-pushed
the
eliminate-copy-response
branch
5 times, most recently
from
31c307e to
a379730
Compare
flutter-dashboard
bot
added
platform-android
platform-fuchsia
platform-ios
platform-macos
labels
|
I filed a bug for the fuchsia failure: https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=102798 Since it works in darwin and android it seems to be a fuchsia bug. We can cut them out of this change for now. |
gaaclarke
force-pushed
the
eliminate-copy-response
branch
2 times, most recently
from
dd4e890 to
394980c
Compare
There was a problem hiding this comment.
I don't think this approach is safe for the following reasons:
- If the size of the buffer is changed from Dart code, there is no way to resize the underlying mapping.
- For files in read-only storage, you simply cannot get mappings that are read-write. You will get a MAP_FAILED in these circumstances. This is probably what is happening on Fuchsia and Windows. I expected the same thing to happen on iOS too since the assets are in read-only storage too. Perhaps this is in debug mode where the files are synced to writable storage? Not sure. This probably doesn't work on iOS in release mode.
- On platforms where you can get a mutable mapping (desktops perhaps), applications can now change the files on disk because they can write to mappings they get from platform message responses. This is unexpected to them because they never realized the mappings were backed by files. Oh, and even that is not guaranteed to work because there is no deterministic mflush.
- You are relying on dodgy pointer const casts and the implementation of NSData on iOS.
The way I read this, this doesn't work on Android, Fuchsia, Windows and probably iOS (really, any platform that has a read-only asset pool), relies on pointer casts and implementation details on Foundation, doesn't handle buffer resizes, allows applications to unexpected change assets on disk where they can get a mapping, and makes the FML Mappings a whole lot more complicated.
For these reasons, I don't think this is the right approach. I would rather have another interface that gives the Dart code a read only mapping and perform a migration to that API. I suspect that is what 99% of the use cases want anyway.
|
Dart can't resize bytedata, but the other points stand |
|
I agree with #34018 (review). Returning a Dart ByteData backed by a copy-on-write file mapping will produce unexpected behavior. Is there a specific application scenario that this is targeting? If the goal is to improve the performance of platform messages sent by host applications, then it might be worthwhile to avoid an extra copy if the message is backed by a malloced buffer or std::vector. If the issue is related to loading assets, then we probably need a separate Dart API that represents a read-only mapping (as mentioned above). |
|
Thanks for taking a look @chinmaygarde @dnfield @jason-simmons . I'll try to respond to all the feedback. There are 3 optimizations here: assets, platform channels on ios and platform channels on android. No one has a problem with the android platform channels, I'll split my feedback between the optimizations. Assets
As Dan pointed out, there is no API to do that. It couldn't be safely done by the Dart VM either since it doesn't know how the memory was created.
I think there is a misunderstanding about how mmap works with copy-on-write. The flag that creates copy-on-write semantics is MAP_PRIVATE which means all the modifications to the data are private to the process, the files on disk are not modified so the file permissions don't matter (more info). The unit test I've provided asserts this by loading the contents in memory again after the memory is edited. The way this works is the OS will switch a VM page from looking at disk to looking at a page that is specific to the process (probably another region of RAM). This is possible on Windows too, see the documentation on FILE_COPY_MAP. It's just a notoriously finicky API and the engine requires a lot of effort getting development setup on Windows, that's why I didn't tackle it up yet.
This isn't the case because of the previous point. iOS Platform Channels
The reason here is because it is mostly* safe. In most cases those NSData objects are ephemeral data blobs that we are creating (example). Unfortunately objc lacks the the ability to statically assert that we have one unique owner of the NSData, but that would be the case. When you have one unique owner, reinterpretting const memory to non-const memory is safe. *) I say mostly safe because there is one case where it could be problematic now that I think of it. If a user is using a BinaryCodec and responds to a message with NSData, then Dart modifies that data, then it is read again in in objc that would be a problem. This could be solved by querying the Codec if it copies the data when responding and inserting a copy if it isn't. If you have concerns beyond that we can address them with tests. Android Platform Channels
That's what's happening in the Android case, we can statically assert we have one owner of the read-write data so we are good here. |
|
Here's local testing results of the platform channel benchmarks for Android on a Pixel 4a beforeafterLooks like it is slightly slower for small payloads and 30% faster for 1MB payloads. |
|
I've removed the iOS implementation since it will take a bit more work to get it working safely with the BinaryCodec (or custom codecs). I'll land it in a separate PR. |
|
Is it beneficial to trade off the performance of small messages for larger ones? |
The previous implementation was already conditionalizing how data was being treated based on its size for performance reasons: https://github.com/gaaclarke/engine/blob/a01f69234e9cc0d21d4a972e4e22bb19737e00fe/third_party/tonic/typed_data/dart_byte_data.cc#L26:L26 I can duplicate that logic here. (sorry, i edited your comment on accident instead of replying) |
|
@jonahwilliams I duplicated that logic in the latest patch and here are the results for local testing (on Android pixel 4a): |
gaaclarke
force-pushed
the
eliminate-copy-response
branch
from
20ba04a to
343c07b
Compare
|
@chinmaygarde friendly ping since my response was right before a long weekend. |
Chat summary@chinmay and I had a discussion about this issue last night on Discord. Here was the takeaways, feel free to correct me if I'm misconstruing something.
Options to move forwardHere's some options I've thought of that address the concerns. Mapping->Dart ConversionPromoting class Mapping {
// Move the Mapping to be wrapped in a thread-safe Dart
// ByteBuffer. If this call is successful, calling
// "GetMapping()" afterwards will return nullptr.
virtual std::optional<Dart_Handle> MoveToByteBuffer() =0;
};The benefit of this is that this method cannot be used for a purpose beyond what we need it for and thus making it easier to modify in the future. It could be converted to edit: We could also make the function always return something, incurring a copy if it has to. Putting that logic in the Mapping instead of in the handler. VisitorIf we are concerned about modifying the Mapping API we can add the functionality we need with the Visitor design pattern. class MappingVisitor {
virtual void Visit(FileMapping* mapping) =0;
virtual void Visit(MallocMapping* mapping) =0;
//...
};
class Mapping {
virtual void Visit(MappingVisitor* visitor) =0;
};That way we can write the logic to decide if we can transfer ownership to Dart inside of a Visitor. The downside to this is there are subclasses of Migrate to PlatformMessageResponse to MallocMappingMigrating the response class to using MallocMapping would allow us to remove the copy in certain cases without modifying the class PlatformMessageResponse {
virtual void Complete(std::unique_ptr<fml::MallocMapping> data) =0;
};The major downside of this though is that it may actually introduce an extra copy in the case where the payload is small so we are performing a copy when passing the data to Dart and the payload wasn't already in a MallocMapping. For example, iOS Platform Channels whose response payload is less than 1000 bytes. |
| std::initializer_list<fml::FileMapping::Flags>()); | ||
| #else | ||
| // We use a copy-on-write mapping so we can safely pass ownership to Dart | ||
| // which currently lacks immutable data buffers. |
There was a problem hiding this comment.
Dart has them, but the implementation is problematic:
https://github.com/dart-lang/sdk/blob/main/sdk/lib/typed_data/unmodifiable_typed_data.dart. The problem is that including them causes the regular typed data classes to become non-monomorphic, which breaks inlining optimizations. Might be worth benchmarking with anyway.
|
One other thought that I didn't communicate well. The data we are actually sending here in a response is one of 4 types:
Only type 4 requires a Dart representation that is immutable. It also represents a fraction of the actual usage. Right now it is iOS platform channel usage but I think we can use convention and tests to capture most of iOS's use cases as well by proving that the messenger has unique ownership. |
|
Here is some data supporting the importance of this change. (@zanderso @jonahwilliams @jiahaog ) Microbenchmarks SummaryHere is a reminder of the microbenchmarks results:
Customer MoneyHere is some data that shows the potential gain for customer money. Sqflite latency breakdownIn b/237035671 customer money asks why the wall time for loading data from sqflite takes 17ms when the actual execution time of java handler takes 1ms. The reasons why are:
I've asked them measure the size of their responses to understand what portion of the 16ms is the response copy. Platform channel auditWe do have data from customer money that gives us understanding about how platform channels are being used since the savings from this PR are a function of the size of the response payloads. Since this improvement is gated on a payload greater than 1000 bytes, here are the platforms channels reported by customer money that are communicating over 1000 bytes on the "downstream" (in the direction of Host->Flutter):
When we hear back from customer money about the individual sizes we can make sure some of these payloads are over 1000 bytes. Asset loading at startupI also audited asset loading on customer money's prototype app on iOS and these are the assets getting loaded at startup and their size (remember anything over 1000 bytes would receive an improvement):
We cannot easily say how much time this PR would save the app at startup, or generalize for every possible app because it depends on how parallelized the code is and the system is. We'll get a better idea if we can get them to test the PR. |
|
@jiahaog may know the most up-to-date way to try out this patch. Suggest following up offline. |
|
I've audited the time spent performing this copy of data in customer money's prototype app on iOS at startup as well. There were 20 instances of the copy called with a total time of 7ms on an iPhone SE 2 (this is the copy that is removed in this PR for iOS assets): profile patch--- a/third_party/tonic/typed_data/dart_byte_data.cc
+++ b/third_party/tonic/typed_data/dart_byte_data.cc
@@ -8,6 +8,18 @@
#include "tonic/logging/dart_error.h"
+#include <os/signpost.h>
+
+static os_log_t points_of_interest_logger = NULL;
+
+static os_log_t GetPointsOfInterestLogger() {
+ if (!points_of_interest_logger) {
+ points_of_interest_logger =
+ os_log_create("dev.flutter", OS_LOG_CATEGORY_POINTS_OF_INTEREST);
+ }
+ return points_of_interest_logger;
+}
+
namespace tonic {
namespace {
@@ -28,9 +40,17 @@ Dart_Handle DartByteData::Create(const void* data, size_t length) {
// The destructor should release the typed data.
return handle;
} else {
+ static uint64_t counter = 1;
+ static uint64_t identifier = counter++;
+ if (__builtin_available(macOS 10.14, iOS 12.0, *)) {
+ os_signpost_interval_begin(GetPointsOfInterestLogger(), identifier, "DartByteData memcpy");
+ }
void* buf = ::malloc(length);
TONIC_DCHECK(buf);
::memcpy(buf, data, length);
+ if (__builtin_available(macOS 10.14, iOS 12.0, *)) {
+ os_signpost_interval_end(GetPointsOfInterestLogger(), identifier, "DartByteData memcpy");
+ }
return Dart_NewExternalTypedDataWithFinalizer(
Dart_TypedData_kByteData, buf, length, buf, length, FreeFinalizer);
}
|
|
@chinmaygarde friendly ping. This is caught up on your feedback. I responded to it with some options to move forward. Can you please capture where your head is at now. I'm back from vacation now and can fill in any gaps, but it seems like discussion on his PR usually comes back to people trying to interpret where your thoughts are now. FWIW @zanderso told me that const buffers in Dart should be coming this quarter so keep that in mind that there seems to be movement there. |
gaaclarke
force-pushed
the
eliminate-copy-response
branch
from
343c07b to
ac5c3e7
Compare
|
Rebased to help @jiahaog's testing. |
|
Update: The Dart team believes they may have a way to present a const bytebuffer with little runtime performance. Previously they thought the cost to reading and writing would make it not worth it because of some global polymophism optimization that is currently happening. We got the latests results from |
|
The unmodifiable bytes change for Dart has landed: dart-lang/sdk@c1e67ac I haven't had a chance to hook it up. This looks like we can make it a non-breaking change compile-wise since UnmodifiableByteDataView implements ByteData (dart-lang/sdk@c1e67ac#diff-ff924e00bfbb53afcdf9b21700dd244df23776793371f29db6c46c47b8987b0fR5487). It will still technically be a breaking change at runtime (that I suspect no one will run into). I've been asked by the Dart team to check out the performance ramifications too. |
gaaclarke
changed the title
|
closed in lieu of #35468 |
This makes
platform_channel_basic_binary_2host_1MBgo from 529.2 µs to 412.4 µs (22% improvement) in my local testing.The test is in the platform channel benchmarks in the flutter repo.
I found this optimization when exploring asset loading, so it should be beneficial there as well by eliminating a copy.
WARNING: Do not merge until flutter/flutter#105982 merges in the framework.
local testing results
Executed on iOS device.
before
after
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.