Sped up the objc standard message codec #25998

gaaclarke · 2021-05-07T00:23:46Z

DO NOT MERGE UNTIL WE VERIFY THAT THE PLATFORM CHANNEL BENCHMARKS ARE RUNNING

This improves performance of the performance benchmarks ~5%, nothing huge but since I went through the exercise it's worth capturing the gains. The biggest gains were from caching the classes. objc_msgSend was showing up in the profiler but switching to c functions didn't give the increase I was hoping for.

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.

flutter-dashboard · 2021-05-07T00:23:48Z

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.

jmagman

Which performance benchmarks did this impact?

Can you add some comments, I can picture someone reverting all this because they don't understand that you're avoiding obj message calls.

jmagman · 2021-05-07T19:49:02Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodec.mm

  }
 }
+
+- (void)writeByte:(UInt8)value {
+  WriteByte((CFMutableDataRef)_data, value);


jmagman · 2021-05-07T19:50:06Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodec.mm

@@ -6,6 +6,15 @@

 #pragma mark - Codec for basic message channel

+static const UInt8 s_zeroBuffer[8] = {0, 0, 0, 0, 0, 0, 0, 0};


Can you move these statics into the functions where they are called to make it more obvious?

You may want to put the initialization in a dispatch_once block in the functions so they aren't set until they are needed.

I've gotten comments asking for keeping them at the top in other code reviews at google. I double checked the style guide and I couldn't find any recommendation either way. These won't actually get set until they are paged into memory.

jmagman · 2021-05-07T19:55:35Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodec.mm

      }
    }
    if (!success) {
      NSLog(@"Unsupported value: %@ of number type %ld", value, CFNumberGetType(number));
-      NSAssert(NO, @"Unsupported value for standard codec");
+      assert(NO);  // Unsupported value for standard codec.


Can we leave the NSAssert since the nicer NSAssertionHandler behavior with file and line numbers, etc would be more important than performance at the point it fails?

You can't use NSAssert outside of objective-c methods, it's a macro that depends on having access to self.

Oh right, NSCAssert then.

ahh yea, forgot about that, thanks, done

jmagman · 2021-05-07T19:57:46Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodec.mm

@@ -221,115 +230,142 @@ - (void)dealloc {
  [super dealloc];
 }

- (void)writeByte:(UInt8)value {
-  [_data appendBytes:&value length:1];
+static void WriteByte(CFMutableDataRef data, UInt8 value) {


Should this have a FLT prefix?

It's not required because these are static functions, their visibility is limited to this compilation unit so they can't collide.

gaaclarke · 2021-05-07T20:37:21Z

After reviewing the style guide for the location of constants question it reminded me that the prefix should be k for constants, so I switched them all over.

jmagman

I think this could still use a comment to say why you are caching the classes in statics since it's unexpected.
Other than that, if it's a 5% improvement, LGTM.

gaaclarke · 2021-05-12T17:52:31Z

Ugh, I wasn't suppose to merge this until the performance tests were running. I left for vacation then overlooked my own warning lol. Hopefully they are running before this rolls.

* b629338 Roll Skia from c1b6b6c615a7 to d9a7c5953df3 (2 revisions) (flutter/engine#26089) * 5d67ad1 Get licenses check deps from gclient rather than pub (flutter/engine#26077) * b2104fa Roll Dart SDK from d59cb89f73fe to 934cc986926d (1 revision) (flutter/engine#26090) * b825396 Roll Fuchsia Linux SDK from 1PbnXEErn... to MoY7UVVro... (flutter/engine#26091) * 2b5b0b8 Roll Skia from d9a7c5953df3 to 827bb729a84d (2 revisions) (flutter/engine#26093) * e834ec3 Remove "unnecessary" imports. (flutter/engine#25736) * da5aaf8 Roll Skia from d9a7c5953df3 to 827bb729a84d (2 revisions) (flutter/engine#26096) * e13e345 Roll Skia from 827bb729a84d to 433d25c947e4 (3 revisions) (flutter/engine#26097) * 99021da Sped up the objc standard message codec (flutter/engine#25998) * d7229e9 Roll Skia from 433d25c947e4 to 0270bf5d10be (5 revisions) (flutter/engine#26100) * a339374 Move more parts to libs. Move bitmap/dom canvas to /html (flutter/engine#26072)

This appears to have triggered reproducible failures in channels_integration_test_ios: [ +4 ms] 00:01 [32m+0[0m: channel suite step through[0m [+3744 ms] Unsupported value: Sun Mar 11 07:16:42 2018 of type __NSTaggedDate [ ] *** Assertion failure in void WriteValue(CFMutableDataRef, id)(), FlutterStandardCodec.mm:340 [ +2 ms] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Unsupported value for standard codec.' [ ] *** First throw call stack: [ ] (0x19d5bd9d8 0x1b1940b54 0x19d4cc50c 0x19e815238 0x1050031ec 0x104823f80 0x105003aac 0x1050009bc 0x104824e9c 0x105000b4c 0x104d0cc98 0x10501b398 0x104fb3c94 0x104fb72c4 0x19d53e3e0 0x19d53dfe4 0x19d53d4c4 0x19d537850 0x19d536ba0 0x1b429c598 0x19fe282f4 0x19fe2d874 0x1048257fc 0x19d215568) [ ] libc++abi.dylib: terminating with uncaught exception of type NSException [ +65 ms] Process 541 stopped [ ] * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT [ ] frame #0: 0x00000001c93e584c libsystem_kernel.dylib`__pthread_kill + 8 [ ] libsystem_kernel.dylib`__pthread_kill: [ ] -> 0x1c93e584c <+8>: b.lo 0x1c93e5868 ; <+36> [ ] 0x1c93e5850 <+12>: stp x29, x30, [sp, #-0x10]! [ ] 0x1c93e5854 <+16>: mov x29, sp [ ] 0x1c93e5858 <+20>: bl 0x1c93c2f5c ; cerror_nocancel [ ] Target 0: (Runner) stopped. Example builds: * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/828/overview * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/829/overview * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/830/overview * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/831/overview Example Log: * https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8847377106855056784/+/u/run_channels_integration_test_ios/stdout This reverts commit 99021da.

cbracken · 2021-05-13T05:09:23Z

Rollback is #26117.

This appears to have triggered reproducible failures in channels_integration_test_ios: [ +4 ms] 00:01 [32m+0[0m: channel suite step through[0m [+3744 ms] Unsupported value: Sun Mar 11 07:16:42 2018 of type __NSTaggedDate [ ] *** Assertion failure in void WriteValue(CFMutableDataRef, id)(), FlutterStandardCodec.mm:340 [ +2 ms] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Unsupported value for standard codec.' [ ] *** First throw call stack: [ ] (0x19d5bd9d8 0x1b1940b54 0x19d4cc50c 0x19e815238 0x1050031ec 0x104823f80 0x105003aac 0x1050009bc 0x104824e9c 0x105000b4c 0x104d0cc98 0x10501b398 0x104fb3c94 0x104fb72c4 0x19d53e3e0 0x19d53dfe4 0x19d53d4c4 0x19d537850 0x19d536ba0 0x1b429c598 0x19fe282f4 0x19fe2d874 0x1048257fc 0x19d215568) [ ] libc++abi.dylib: terminating with uncaught exception of type NSException [ +65 ms] Process 541 stopped [ ] * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT [ ] frame #0: 0x00000001c93e584c libsystem_kernel.dylib`__pthread_kill + 8 [ ] libsystem_kernel.dylib`__pthread_kill: [ ] -> 0x1c93e584c <+8>: b.lo 0x1c93e5868 ; <+36> [ ] 0x1c93e5850 <+12>: stp x29, x30, [sp, #-0x10]! [ ] 0x1c93e5854 <+16>: mov x29, sp [ ] 0x1c93e5858 <+20>: bl 0x1c93c2f5c ; cerror_nocancel [ ] Target 0: (Runner) stopped. Example builds: * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/828/overview * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/829/overview * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/830/overview * https://ci.chromium.org/ui/p/flutter/builders/prod/Mac_ios%20channels_integration_test_ios/831/overview Example Log: * https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8847377106855056784/+/u/run_channels_integration_test_ios/stdout This reverts commit 99021da.

flutter-dashboard bot added platform-ios platform-macos labels May 7, 2021

google-cla bot added the cla: yes label May 7, 2021

gaaclarke requested a review from jmagman May 7, 2021 00:23

Sped up the StandardMessageCodec for iOS, tiny tweaks about 5% gain.

678f4e7

gaaclarke force-pushed the faster-standard-codec branch from 09638ac to 678f4e7 Compare May 7, 2021 16:40

jmagman reviewed May 7, 2021

View reviewed changes

gaaclarke added 2 commits May 7, 2021 13:32

added __bridge and renamed constants

763f563

switched to nscassert

8f93af8

gaaclarke requested a review from jmagman May 7, 2021 20:37

jmagman approved these changes May 7, 2021

View reviewed changes

added comment

18d4b10

gaaclarke merged commit 99021da into flutter:master May 12, 2021

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

Roll Engine from f3847febb247 to 99021da09058 (9 revisions) flutter/flutter#82379

Closed

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

99021da Sped up the objc standard message codec (flutter/engine#25998)

5ca8e8a

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

Roll Engine from f3847febb247 to d7229e95d24a (10 revisions) flutter/flutter#82381

Closed

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

99021da Sped up the objc standard message codec (flutter/engine#25998)

419593a

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

Roll Engine from f3847febb247 to a33937429616 (11 revisions) flutter/flutter#82388

Merged

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

99021da Sped up the objc standard message codec (flutter/engine#25998)

7e7bc29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sped up the objc standard message codec #25998

Sped up the objc standard message codec #25998

gaaclarke commented May 7, 2021 •

edited

Loading

flutter-dashboard bot commented May 7, 2021

jmagman left a comment

jmagman May 7, 2021

gaaclarke May 7, 2021

jmagman May 7, 2021

gaaclarke May 7, 2021

jmagman May 7, 2021

gaaclarke May 7, 2021

jmagman May 7, 2021

gaaclarke May 7, 2021

jmagman May 7, 2021

gaaclarke May 7, 2021

gaaclarke commented May 7, 2021

jmagman left a comment

gaaclarke commented May 12, 2021

cbracken commented May 13, 2021

		@@ -6,6 +6,15 @@

		#pragma mark - Codec for basic message channel

		static const UInt8 s_zeroBuffer[8] = {0, 0, 0, 0, 0, 0, 0, 0};

Sped up the objc standard message codec #25998

Sped up the objc standard message codec #25998

Conversation

gaaclarke commented May 7, 2021 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented May 7, 2021

jmagman 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

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

gaaclarke commented May 7, 2021

jmagman left a comment

Choose a reason for hiding this comment

gaaclarke commented May 12, 2021

cbracken commented May 13, 2021

gaaclarke commented May 7, 2021 •

edited

Loading