Removed superfluous copy operations in darwin flutter platform messages by wrapping data in custom NSData classes. #25702

gaaclarke · 2021-04-21T23:51:52Z

Renamed methods that copy data from Get* to Copy*
Removed a few instances where we are doing an extra copy of data by using a proxy object that conforms to NSData but holds on the underlying data.

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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2021-04-21T23:55:26Z

shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm

+}
+
+ (NSData*)dataWithMapping:(std::unique_ptr<fml::Mapping>)mapping {
+  return (NSData*)[[[FlutterMappingData alloc] initWithMapping:std::move(mapping)] autorelease];


We could selectively just copy the data here if the buffer is small enough.

gaaclarke · 2021-04-21T23:55:52Z

shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm

+
+    // Const cast is required because the NSData API requires it despite
+    // guarentees that the buffer won't be deleted or modified.
+    _data = [[NSData alloc] initWithBytesNoCopy:const_cast<void*>(rawData)


This is an extra allocation that wouldn't be happening before in lieu of a copy.

gaaclarke · 2021-04-22T00:07:54Z

@stuartmorgan @xster I was trying to figure out how platform channels work completely. When looking into it I saw we were doing an extra copy of the data to get NSData's for passing to the user. This PR removes two of those extra copies.

We've had some users complain about the performance of sending large buffers so this will help with that (@xster do we have an issue open for that?). This technically could be slower if the buffers are small since it has an extra allocation and one layer of indirection when accessing the bytes. We could get around that by selectively copying if the buffers are small. We'd have to do some microbenchmarks to get an idea about the math for that.

While looking I've also found an opportunity to speed up android platform channels by using direct buffers. That sounds worthwhile, what do you think? We could also potentially remove the copies that are happening in CopyNSDataToVector and CopyNSDataToMapping if we transitioned away from using std::vector and instead used our own buffer wrapper that can do no-copy initializations and releases. That might be a bit more involved though.

xster · 2021-04-22T06:05:08Z

While looking I've also found an opportunity to speed up android platform channels by using direct buffers. That sounds worthwhile, what do you think?

Yes I think so! The users that brought this issue up were testing on Android.

stuartmorgan-g · 2021-04-22T14:53:19Z

shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm


+/// A proxy object that behaves like NSData represented in a Mapping.
+/// This isn't a subclass of NSData because NSData is in a class cluster.
+@interface FlutterMappingData : NSObject


Why is this inheriting from NSObject rather than NSProxy?

Because NSObject gives us forwardingTargetForSelector, the NSProxy is slower. Check out the documentation on forwardingTargetForSelector https://developer.apple.com/documentation/objectivec/nsobject/1418855-forwardingtargetforselector

stuartmorgan-g · 2021-04-22T14:59:00Z

shell/platform/darwin/ios/framework/Source/platform_message_response_darwin.mm

+- (id)forwardingTargetForSelector:(SEL)aSelector {
+  return _data;
+}
+


It's been a while since I've written a proxy object, but IIRC without several other method overrides (e.g., respondsToSelector), this won't actually look like an NSData in various circumstances.

You might argue that those are edge cases, but this is being passed to arbitrary third-party code, and claims to be an NSData; those edge cases are all valid, and we have no way to guarantee that they don't exist.

The respondsToSelector+forwardInvocation is a different slower forwarding mechanism.

Yea, if the code ever did reflection on the NSData it could be a problem. Subclassing class clusters isn't clear, they really push forwarding in the documentation.

Correctness is more important that speed though, and I find it very unlikely that the cost of calls to this "NSData" object is going to even more than a tiny, tiny fraction of the end-to-end performance of platform channels.

(Edited to add: to be clear, by "cost of calls" I mean the actual cost of doing the method invocation, including forwarding, not the overall operations like the copy you are trying to eliminate.)

I was mistaken, because NSData is a class cluster this isn't actually a problem. Reflection doesn't work on proper NSData classes anyways. If you do the following:

#include <Foundation/Foundation.h> int main() { NSData* data = [[NSData alloc] init]; NSLog(@"%@", data.class); }

it prints out _NSZeroData. You can't do reflection on NSData so this is as correct as can be.

That said, I'm still going to investigate the alternative we discussed since it is cleaner.

You can't do reflection on NSData

You've demonstrated one specific call. What about isKindOfClass:? And much more importantly, how does your implementation handle respondsToSelector?

Yea, I'd have to override respondsToSelector. To get it to respond to isKindOfClass: I'd have to either override all methods, or fiddle with the objc_class. I put a pin in this while I research the broader question: https://docs.google.com/document/d/1oNLxJr_ZqjENVhF94-PqxsGPx0qGXx-pRJxXL6LSagc/edit

stuartmorgan-g · 2021-04-22T15:02:43Z

shell/platform/darwin/ios/framework/Source/platform_message_router.mm

    NSData* data = nil;
    if (message->hasData()) {
-      data = GetNSDataFromVector(message->data());
+      data = [FlutterMessageData dataWithMessage:std::move(message)];


It seems like it would be much safer to change PlatformMessage to allow for extracting the data with ownership, so we could actually just construct a no-copy NSData here, rather than using a proxy.

Yea, you're probably right. I'll have to investigate, they are using shared pointers for the platformmessage, but I think they are essentially unique_ptr's. If they are truly unique_ptr's we can steal ownership of the buffer. It might help us on the android side too. I can look into this today

gaaclarke · 2021-05-04T00:09:16Z

@stuartmorgan Okay, I spent some time researching this and trying out what we discussed. This is the right approach, the alternative is a massive breaking change: #25867. While that approach is a good idea and would allow us to eliminate all 4 copies of the message data, it percolates all the way up to shell/platform/common/client_wrapper/include/flutter/message_codec.h where we are using std::vector, which is a pretty big deal.

This approach will eliminate 3/4 copies and is much less invasive/risky/controversial. I've implemented performance tests so I recommend we do this and reevaluate if we want to do the breaking change to geek out the last copy. (edit: the remaining copy that couldn't be reviewed is in the Host->Flutter direction for Host originating messages, so it doesn't affect plugins fwiw).

I implemented the respondsToSelector for our NSData classes and read through more Apple documentation. Here is what it says about isKindOfClass:: "Be careful when using this method on objects represented by a class cluster. Because of the nature of class clusters, the object you get back may not always be the type you expected. If you call a method that returns a class cluster, the exact type returned by the method is the best indicator of what you can do with that object." (link. So, this behavior is consistent with class clusters.

I believe we'll be able to do something similar for JNI, too. I don't think the Mapping breaking change is needed to eliminate the 3 copies on Android either.

gaaclarke · 2021-05-04T00:25:42Z

shell/platform/darwin/common/buffer_conversions.h

 #include <vector>

 #include "flutter/fml/mapping.h"
+#import "flutter/lib/ui/window/platform_message.h"


This could be removed if my other PR lands where I migrate PlatformMessage to std::unique_ptr, then I could steal its std::vector instead of wrapping the PlatformMessage.

flutter-dashboard · 2021-05-04T00:30:27Z

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.

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.

gaaclarke · 2021-05-04T22:42:08Z

I've been able to measure this now that we have performance tests and it doesn't actually make anything faster. For standard message codecs it seems the serialization time dwarfs copying time. For binary codecs the majority of time is in the deallocator for the std::vector. I'm not sure why yet, but regardless this PR isn't worth considering until it can prove it can deliver performance, dropping back to draft.

gaaclarke · 2021-05-07T22:08:53Z

Okay, officially closing this, choosing to go down the mapping route: #25867
I took out the pieces of this that I needed to eliminate the copy here: #25988

gaaclarke · 2021-05-07T22:09:47Z

FWIW I profiled the other fix and it seems like eliminating the copy didn't show up in 14k payloads, but it did in 1mb payloads. The mapping also had the added benefit of eliminating std::vector deallocation which was heavy.

flutter-dashboard bot added platform-ios platform-macos labels Apr 21, 2021

google-cla bot added the cla: yes label Apr 21, 2021

gaaclarke commented Apr 21, 2021

View reviewed changes

stuartmorgan-g suggested changes Apr 22, 2021

View reviewed changes

Removed superfluous copy operations in iOS flutter platform messages.

5d01db9

gaaclarke force-pushed the remove-platform-channel-copies branch from 1c941ac to 5d01db9 Compare May 3, 2021 23:42

gaaclarke added 2 commits May 3, 2021 16:42

some cleanup and moved the objc classes to buffer_conversions

b6c339f

removed another copy when going from NSData to Mapping

bf68af7

gave buffer_conversions access to platform messages

2017443

gaaclarke commented May 4, 2021

View reviewed changes

gaaclarke marked this pull request as ready for review May 4, 2021 00:30

gaaclarke requested a review from stuartmorgan-g May 4, 2021 00:30

removed assert

44edb68

gaaclarke marked this pull request as draft May 4, 2021 22:42

gaaclarke changed the title ~~Removed superfluous copy operations in darwin flutter platform messages.~~ Removed superfluous copy operations in darwin flutter platform messages by wrapping data in custom NSData classes. May 5, 2021

gaaclarke closed this May 7, 2021

Removed superfluous copy operations in darwin flutter platform messages by wrapping data in custom NSData classes. #25702

Removed superfluous copy operations in darwin flutter platform messages by wrapping data in custom NSData classes. #25702

Uh oh!

Conversation

gaaclarke commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xster commented Apr 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented May 4, 2021

Uh oh!

gaaclarke commented May 4, 2021

Uh oh!

gaaclarke commented May 7, 2021

Uh oh!

gaaclarke commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Apr 21, 2021 •

edited

Loading

gaaclarke commented Apr 22, 2021 •

edited

Loading

stuartmorgan-g Apr 22, 2021 •

edited

Loading

gaaclarke commented May 4, 2021 •

edited

Loading

gaaclarke May 4, 2021 •

edited

Loading