Switch PlatformMessages to hold data in Mappings #25867

gaaclarke · 2021-04-30T23:29:18Z

Switch PlatformMessages to having their data in std::vector to fml::Mapping so that we can steal ownership of the data and have more efficient allocation / deallocation of the data.

On iOS this had about a 20% increase in performance tests for 14k binary platform messages. The savings seem to come from the removal of the ~std::vector<uint8_t> calls.

Do not merge this until the performance tests are running.
This PR depends on #25860
Next PR: #25988
Issue: flutter/flutter#81559

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-05-04T23:14:42Z

I measured this locally with the unpublished platform channel tests, without removing the copy, just transitioning away from std::vector<uint8_t>.

For binary codecs it's a 21% reduction (.193ms -> .1523ms). For standard message codecs it's a 4% reduction (.13048ms -> .12524ms).

…::vector

…ion anyway)

chinmaygarde

Only major comment is about the error handling (OOB access and OOMs) and documentation. Other than that, everything else are minor nits. This is great. I have always been uneasy using vectors for bags of bytes.

chinmaygarde · 2021-05-07T20:06:14Z

fml/mapping.h

+  /// It's templated since void* arithemetic isn't allowed and we want support
+  /// for `uint8_t` and `char`.
+  template <typename T>
+  static MallocMapping Copy(const T* begin, const T* end) {


This needs some error handling to ensure we don't read OOB. Specifically, begin > end and end > begin + size. The return can be an MallocMapping.

A suggestion: A variant I have found super useful in other projects is the following:

template <class T> RetVal Copy(const T* buffer, size_t count) { return Copy(reinterpret_cast<const uint8_t*>(buffer), count * sizeof(buffer)); };

Which is an effective way of saying copy count objects of type T tightly packed in this buffer. If you want extra safeguards in place, you can even std::enable_if the specialization to ensure that T is std::is_trivially_constructible. That way, folks don't accidentally end up copying non-POD types.

Done, I can't assert on size since it doesn't exist here. The reason I chose this signature was just because it matched the code I was migrating from std::vector. In the end I think I moved everything to the Copy signature with length, we could probably delete this but since it was helpful before I thought it might be helpful in the future.

chinmaygarde · 2021-05-07T20:07:14Z

fml/mapping.h

+
+  MallocMapping(const uint8_t* data, size_t size);
+
+  MallocMapping(fml::MallocMapping&& mapping);


Is there a reason for the preference for this to be allocatable on the stack instead of using unique_ptr to own MallocMappings? Either way, the allocation is going to be on the heap.

I didn't think about it much beyond just trying to keep it as close as possible to std::vector to make migration easier. I guess this is slightly more efficient since you can read the size without the indirection.

chinmaygarde · 2021-05-07T20:08:19Z

fml/mapping.cc

+}
+
+MallocMapping MallocMapping::Copy(const void* begin, size_t length) {
+  auto result =


Error handling for OOM cases on malloc failures.

chinmaygarde · 2021-05-07T20:10:05Z

fml/mapping.h

+  ~MallocMapping() override;
+
+  /// Copies the data from `begin` to `end`.
+  /// It's templated since void* arithemetic isn't allowed and we want support


Not sure I understand this reasoning. void * arithmetic is disallowed but the caller can just reinterpret_cast it though right?

Yep, they could. I was trying to make the migration as easy as possible, just added what I made it easiest. The problem was that we use uint8_t everywhere but when you are creating mappings from strings they use int8_t.

chinmaygarde · 2021-05-07T20:12:00Z

fml/mapping.h

+    return Copy(begin, length);
+  }
+
+  static MallocMapping Copy(const void* begin, size_t length);


Docset for the signature. Specifically mentioning the following:

The error handling to be expected. What happens on OOM and out of bounds access on the source buffer for instance.

Clarify that size is in bytes.

Suggestion: If this were const uint8_t*, the length parameter becomes much clearer.

Done, I used void* in order to support uint8_t and int8_t. Otherwise I'd have to use templates like above.

chinmaygarde · 2021-05-07T20:13:43Z

fml/mapping.h

+  const uint8_t* GetMapping() const override;
+
+  /// Removes ownership of the data buffer.
+  const uint8_t* Release();


I don't think this should be const because the caller won't be able to modify it (for freeing mostly) without a const cast. If this object is relinquishing ownerhip of the buffer, why should it have a say in how the caller uses the buffer that it now owns.

[[nodiscard]] attribute here since ignoring this will be a leak.

Added no discard. Yea, you're right about the constness, remove it.

chinmaygarde · 2021-05-07T20:21:58Z

fml/mapping.cc

+}
+
+MallocMapping::~MallocMapping() {
+  if (data_) {


This is not necessary as ::free(nullptr) is safe. In fact, I think clang-tidy even has a fix-it for this sort of deletion.

Looked it up, its for delete https://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html. Same stuff.

nice, done, thanks

gaaclarke · 2021-05-13T00:13:45Z

@chinmaygarde friendly ping. I have a few more PR's that this depends on.

gaaclarke · 2021-05-13T17:19:19Z

This is good to land once we get the LGTM, the performance tests are running here: https://flutter-flutter-perf.skia.org/e/?queries=test%3Dmac_ios_platform_channels_benchmarks_ios

…ine#25867)

* 7adb8de Roll Skia from 6b719c25cade to ca9f6a855071 (3 revisions) (flutter/engine#26127) * a1062aa [uwptool] Support lookup of full package name (flutter/engine#26121) * 01ce6a1 Get android_lint deps using gclient rather than pub. (flutter/engine#26106) * cba6c1e Use cached CanvasKit instance if available (flutter/engine#26059) * b0bb8ea Switch PlatformMessages to hold data in Mappings (flutter/engine#25867)

flutter-dashboard bot added embedder Related to the embedder API platform-android platform-fuchsia platform-ios labels Apr 30, 2021

gaaclarke changed the title ~~Mapping buffers~~ Switch PlatformMessages to hold data in NonOwnedMapping Apr 30, 2021

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

gaaclarke mentioned this pull request Apr 30, 2021

Moved PlatformMessage's to unique_ptrs #25860

Merged

9 tasks

gaaclarke force-pushed the mapping-buffers branch from 371419a to 0c1007b Compare May 3, 2021 18:04

gaaclarke changed the title ~~Switch PlatformMessages to hold data in NonOwnedMapping~~ Switch PlatformMessages to hold data in Mappings May 3, 2021

flutter-dashboard bot added the platform-macos label May 3, 2021

gaaclarke force-pushed the mapping-buffers branch 8 times, most recently from fccfdb9 to d2bd4f7 Compare May 3, 2021 22:46

gaaclarke mentioned this pull request May 4, 2021

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

Closed

9 tasks

gaaclarke force-pushed the mapping-buffers branch 2 times, most recently from 31ed940 to 2d02b81 Compare May 6, 2021 20:58

gaaclarke mentioned this pull request May 6, 2021

Platform channels, eliminate objc copies #25988

Merged

9 tasks

gaaclarke added 4 commits May 7, 2021 09:13

switched platformmessages to hold fml::NonOwnedMapping instead of std…

dc66ee3

…::vector

switched to MallocMapping

46796d9

added copy that takes in a length (everyone was calculating end posit…

fe80b2a

…ion anyway)

added unit tests for MallocMapping

9aaa52d

gaaclarke force-pushed the mapping-buffers branch from 2d02b81 to 9aaa52d Compare May 7, 2021 16:14

gaaclarke marked this pull request as ready for review May 7, 2021 16:25

gaaclarke requested a review from chinmaygarde May 7, 2021 16:25

chinmaygarde reviewed May 7, 2021

View reviewed changes

chinmays feedback

63da9c4

gaaclarke requested a review from chinmaygarde May 7, 2021 21:23

chinmaygarde approved these changes May 13, 2021

View reviewed changes

gaaclarke merged commit b0bb8ea into flutter:master May 13, 2021

engine-flutter-autoroll mentioned this pull request May 13, 2021

Roll Engine from 917371ffaefe to b0bb8eab1d2f (5 revisions) flutter/flutter#82465

Merged

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

b0bb8ea Switch PlatformMessages to hold data in Mappings (flutter/eng…

fe4cdfb

…ine#25867)


		MallocMapping(const uint8_t* data, size_t size);

		MallocMapping(fml::MallocMapping&& mapping);

Switch PlatformMessages to hold data in Mappings #25867

Switch PlatformMessages to hold data in Mappings #25867

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

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!

chinmaygarde left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented May 13, 2021

Uh oh!

gaaclarke commented May 13, 2021

Uh oh!

Uh oh!

gaaclarke commented Apr 30, 2021 •

edited

Loading

gaaclarke commented May 4, 2021 •

edited

Loading