Eliminates Dart->Host response copy for large buffers #35468
Conversation
gaaclarke
force-pushed
the
eliminate-copies-unmodifiable
branch
from
44ba817 to
32da9e6
Compare
|
@a-siva @rmacnak-google I tested the performance of the UnmodifiableByteDataView, looks good to me. See the results in the description. Thanks! I'll see if I can get the UnmodifiableByteDataView constructor called from c++ to avoid the framework change and put this up for review. |
|
I've tried to eliminate the change in the framework by allocating the UnmodifiableByteDataView in the c++ code but Dart_New crashes with "unreachable" in release builds. edit: This is because UnmodifiableByteDataView is getting tree shaken. void PlatformMessageResponseDart::Complete(std::unique_ptr<fml::Mapping> data) {
PostCompletion(
std::move(callback_), ui_task_runner_, &is_complete_, channel_,
[data = std::move(data)]() mutable {
Dart_Handle byte_buffer;
intptr_t size = data->GetSize();
if (data->GetSize() > 1000) {
// The ByteData will be wrapped with an
// UnmodifiableByteDataView to protect constness in Dart.
void* mapping = const_cast<uint8_t*>(data->GetMapping());
byte_buffer = Dart_NewExternalTypedDataWithFinalizer(
/*type=*/Dart_TypedData_kByteData,
/*data=*/mapping,
/*length=*/size,
/*peer=*/data.release(),
/*external_allocation_size=*/size,
/*callback=*/MappingFinalizer);
} else {
byte_buffer =
tonic::DartByteData::Create(data->GetMapping(), data->GetSize());
}
Dart_Handle type_data = Dart_LookupLibrary(
tonic::DartConverter<std::string>().ToDart("dart:typed_data"));
FML_DCHECK(!(Dart_IsNull(type_data) || Dart_IsError(type_data)));
Dart_Handle unmodifiable_byte_data_type =
Dart_GetNullableType(type_data,
tonic::DartConverter<std::string>().ToDart(
"UnmodifiableByteDataView"),
0, nullptr);
FML_DCHECK(!(Dart_IsNull(unmodifiable_byte_data_type) ||
Dart_IsError(unmodifiable_byte_data_type)));
return Dart_New(unmodifiable_byte_data_type, Dart_Null(), 1,
&byte_buffer);
});
} |
|
I've also attempted adding the unmodifiedbytedataview allocation in the PlatformDispatcher, but it incurs a higher cost. --- a/lib/ui/platform_dispatcher.dart
+++ b/lib/ui/platform_dispatcher.dart
@@ -521,14 +521,17 @@ class PlatformDispatcher {
/// The framework invokes [callback] in the same zone in which this method was
/// called.
void sendPlatformMessage(String name, ByteData? data, PlatformMessageResponseCallback? callback) {
+ PlatformMessageResponseCallback? mutableCallback = callback == null ? null : (ByteData? mutableData) {
+ callback(mutableData == null ? null : UnmodifiableByteDataView(mutableData));
+ };
final String? error =
- _sendPlatformMessage(name, _zonedPlatformMessageResponseCallback(callback), data);
+ _sendPlatformMessage(name, _zonedPlatformMessageResponseCallback(mutableCallback), data);
if (error != null) {
throw Exception(error);
}
} |
|
I tried wrapping the constructor for UnmodifiableByteDataView into a function in dart:ui, this also provides tiny overhead for the standard case as well. void PlatformMessageResponseDart::Complete(std::unique_ptr<fml::Mapping> data) {
PostCompletion(std::move(callback_), ui_task_runner_, &is_complete_, channel_,
[data = std::move(data)]() mutable {
Dart_Handle byte_buffer;
intptr_t size = data->GetSize();
if (data->GetSize() > 1000) {
// The ByteData will be wrapped with an
// UnmodifiableByteDataView to protect constness in Dart,
// see //lib/ui/platform_dispatcher.dart
void* mapping = const_cast<uint8_t*>(data->GetMapping());
byte_buffer = Dart_NewExternalTypedDataWithFinalizer(
/*type=*/Dart_TypedData_kByteData,
/*data=*/mapping,
/*length=*/size,
/*peer=*/data.release(),
/*external_allocation_size=*/size,
/*callback=*/MappingFinalizer);
} else {
byte_buffer = tonic::DartByteData::Create(
data->GetMapping(), data->GetSize());
}
Dart_Handle ui_lib = Dart_LookupLibrary(
tonic::DartConverter<std::string>().ToDart("dart:ui"));
FML_DCHECK(!(Dart_IsNull(ui_lib) || Dart_IsError(ui_lib)));
Dart_Handle result =
Dart_Invoke(ui_lib,
tonic::DartConverter<std::string>().ToDart(
"_wrapUnmodifiableByteData"),
1, &byte_buffer);
FML_DCHECK(!(Dart_IsNull(result) || Dart_IsError(result)));
return result;
}); |
|
Here are the results of selectively wrapping only when we are are using external typed data: |
|
The results between always making the UnmodifiableByteDataView and selectively doing it are so so similar I'm going to go with always creating it to make the behavior more reliable. |
gaaclarke
changed the title
gaaclarke
force-pushed
the
eliminate-copies-unmodifiable
branch
from
32da9e6 to
09acb28
Compare
|
Note the existence of |
Platform Channels are fundamental to regular Flutter execution so I don't believe there will ever be a Flutter app that doesn't use unmodifiable views. Even apps that don't use the public platform channel api are using it indirectly inside of the engine. The benchmarks I used for platform channels never writes to the |
The CL that introduces immutability checks has already landed (see dart-lang/sdk@c1e67ac). Applications that didn't use This PR prevents tree shaking of the unmodifiable class. As a consequence we cannot add a future "If unmodifiable views are tree shaken, never perform immutability checks" - optimization. Though if it's common to have unmodifiable views (some existing g3 apps already use them) - that doesn't matter. We have other (more heavy compiler optimization) opportunities to minimize impact of immutability checks (e.g. by cloning loops for immutable/non-immutable cases)
Agree. Compared with @pragma('vm:entry-point')
ByteData? _wrapUnmodifiableByteData(ByteData? byteData) =>
byteData == null ? null : UnmodifiableByteDataView(byteData);it's much better to make VM API: It will allow safe sharing of immutable bytes across isolates (with this PR we cannot do that because there is mutable access to the backing store of the view (i.e. |
FWIW that isn't necessary for our usage since these ByteData objects are temporal objects used to marshal from the host platform to Dart. I can't guarantee that's always the case since usage of the public api could subvert that, but probably always.
Also FWIW, I went back to the benchmark to make sure there wasn't some regression to platform channels when that CL landed. We are writing to buffers when encoding messages and I didn't see any regression. |
|
Is there an issue filed to add |
done: dart-lang/sdk#49784 |
|
Looks like https://dart-review.googlesource.com/c/sdk/+/255816 landed the new API, can we just use that here? |
👍 yes |
gaaclarke
force-pushed
the
eliminate-copies-unmodifiable
branch
from
09acb28 to
62ae7e8
Compare
Done PTAL. I didn't add the new api to tonic::DartByteData since we don't want to use it without knowing the data is actually const. It don't have the proper context at that point to know if that is the case. |
|
Great! Can you add a test? I'm assuming one that follows this code path and asserts that the byte data is read only or something? |
We already have tests for this this. It doesn't add new functionality, just changes performance. There is a benchmark that tests the new performance in the framework repo: https://github.com/flutter/flutter/blob/5bc4a7612f7fd430933b046aeb9e36a5c7f3916e/dev/benchmarks/platform_channels_benchmarks/lib/main.dart#L236 I'll request an exception from hixie. |
|
Looking at the code, am I right in understanding that the buffer you get in Dart is sometimes mutable and sometimes not, based on how big it is? |
Yep, a previous version made that consistent but got lost when I adopted the new c api. The only guarantee that the Dart API makes is that a |
|
I thought about it a bit more and I think it makes sense to bring back what we had previously and what @Hixie noted. Conditionally being unmodifiable is difficult to test and reason about and by my measurements on iOS the extra cost to wrapping the result is small enough to get lost in the noise of the microbenchmark. |
|
FWIW my worry was that people would depend on it being mutable, and then in some rare edge cases the data would be one byte bigger and their code would throw. I can imagine that would be really difficult to debug. Having the API act consistently across axes the developer isn't thinking of tends to lead to a better developer experience. This suggests a possible test, though: that the buffer behaves the same for small sizes and big sizes. |
gaaclarke
force-pushed
the
eliminate-copies-unmodifiable
branch
from
a15fab1 to
087b3bd
Compare
gaaclarke
added
the
autosubmit
zanderso
left a comment
zanderso
left a comment
There was a problem hiding this comment.
Nice to see this land. Just a suggestion for speeding up small messages.
| } else { | ||
| Dart_Handle mutable_byte_buffer = | ||
| tonic::DartByteData::Create(data->GetMapping(), data->GetSize()); | ||
| Dart_Handle ui_lib = Dart_LookupLibrary( |
There was a problem hiding this comment.
DBC
You may be able to speed this up by using the Dart_PersistentHandle for the library that is already cached in the tonic::DartState object in its tonic::DartClassLibrary field. Then you won't have to allocate the "dart:ui" string on every small message.
I'd suggest also caching the string "_wrapUnmodifiableByteData" in a Dart_PersistentHandle instead of allocating it on every small message. Not sure if tonic has a convenience class for that already, but you can always add a field to flutter::UIDartState if there's a speedup.
There was a problem hiding this comment.
Ahh yeah allocating these strings does add up if this gets chatty. We store a bunch of things like this in platform_configuration.cc.
There was a problem hiding this comment.
Yea, I considered it but didn't know where to put it. FWIW I don't think the wrapping code had a big impact on the microbenchmark, less than 1% of the round trip time for a message. I expected looking up the library to be expensive too but seemed fast. I'll double check everything this afternoon when i go back and look at the official performance numbers.
There was a problem hiding this comment.
The lookup is fast but the string creation adds up.
|
Here are the performance results: For the 1MB payload on Android we have a 22% drop in round trip time, on iOS we have a 36% drop ( https://flutter-flutter-perf.skia.org/e/?numCommits=50&queries=sub_result%3Dplatform_channel_basic_binary_2host_1MB&begin=1661272772 ). For the small payload tests it was hard to tell without better statistics. Now that we have a few runs, comparing the median time for the last 40 runs to the 10 following runs it looks like there is a regression of 11.5% on Android, 7% on iOS. (https://flutter-flutter-perf.skia.org/e/?begin=1661272772&queries=sub_result%3Dplatform_channel_basic_standard_2host_small&requestType=0) I think it's worth looking into caching the wrapping function as Zach suggested. |
|
I tried to cache the closure to invoke but didn't notice any difference in the performance. I looked at the Here is the caching code I was testing: 70f545f I'm going to stop fiddling with it since I can't measure any difference locally. |
6 participants
description
This constitutes a 30% reduction in the 1MB test and little change in the other tests.
This will affect platform channel communications, including loading assets from the asset directory.
This is an alternative to the PR that eliminates copies if we have a modifiable buffer: #34018
breaking change notice
This can break users at runtime in the following cases:
It isn't technically a breaking change since it doesn't appear to break any existing tests.
local testing results
Local iOS platform channel benchmark results with the framework pr flutter/flutter#109707:
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.