Sped up reading with FlutterStandardCodec. #38327

gaaclarke · 2022-12-15T20:47:07Z

No functional change, just an improvement in 19% increase codec reading speed. The gains were using a CFData create function that avoids a copy for a temporary data object until it is converted to a string, avoiding some objc_msgSend, avoiding some retain/release, and avoiding the objc recursion if we know the next object we are reading can be read in C.

before:

after:

tests:

performance benchmark will be covered by the platform_channel_benchmarks
integration tests exist for channels

implementation of investigation @cyanglaz and I did on #37883

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.

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

flutter-dashboard · 2022-12-15T21:55:23Z

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

cyanglaz

LGTM % nits

cyanglaz · 2022-12-15T21:58:38Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodecHelper.c

+    case FlutterStandardFieldFloat64Data:
+    case FlutterStandardFieldList:
+    case FlutterStandardFieldMap:
+    case FlutterStandardFieldFloat32Data:


I don't remember exactly, does the compiler warn you if a new enum is added and a case is not handled in the switch statement?

My point is this might cause mistakes if a new enum is added and this code is not updated. If there is no compiler warning, maybe we can add a comment, or use a different way to determine valid enums.

I moved the check to be right next to the enum declaration and added a comment that it needs to be updated if it changes.

cyanglaz · 2022-12-15T22:37:50Z

shell/platform/darwin/common/framework/Source/FlutterStandardCodecHelper.c

+  if (IsStandardType(type)) {
+    *location += 1;
+    return FlutterStandardCodecHelperReadValueOfType(
+        location, data, type, ReadValue, ReadTypedDataOfType, user_data);
+  } else {
+    return ReadValue(user_data);
+  }
+}


Can we have a unit test in the engine to test this? Basically making sure overriding Standard codec works?
(We have a test in framework, but it would be nice to catch potential bugs in the engine)

good call, done

…d change

…38327)

…117242) * e28b26e1d [linux] Allow overriding asset, ICU data path (flutter/engine#38296) * 35bdb8bfc Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319) * 97e246cb5 Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320) * d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179) * c9e9fa5a9 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323) * a7ec07f13 [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324) * 61e95bacb Remove doc reference to the compute method (flutter/engine#38246) * 353d6949e make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325) * 47a358c5e Started using FlutterEngineGroups by default on Android (flutter/engine#37822) * 7d8e10652 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210) * 8915b81d4 Update buildroot to b2ab6e1. (flutter/engine#38329) * 4fe620643 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (#38320)" (flutter/engine#38331) * 2ff490c1e Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333) * 22251857f Add missing include to FlutterThreadSynchronizer (flutter/engine#38337) * 467cfd7ef Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339) * 010f4ee7a Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340) * 773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#38327) * 70439f606 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341) * bc1647f0d Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342) * cac228aff Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344) * 13ae6eb75 Revert "Started using FlutterEngineGroups by default on Android (#37822)" (flutter/engine#38351) * ed8063861 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352) * dcafebf44 Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343) * 6f6158580 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348) * 0c00bc0a9 Remove 30fps cap from playgrounds (flutter/engine#38347) * 38340bb57 [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346) * 3a6b3f986 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357) * 81b453535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358) * d91e20879 Port touch-based tests from embedder integration test (flutter/engine#38234)

* Sped up reading with FlutterStandardCodec. * added missing license diff * put the IsStandardType in the header so it's closer to where it should change * added unittest for subclassing codecs * fixed lints

…lutter#117242) * e28b26e1d [linux] Allow overriding asset, ICU data path (flutter/engine#38296) * 35bdb8bfc Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319) * 97e246cb5 Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320) * d9580a5e7 Migrate iOS text input plugin to use ARC (flutter/engine#38179) * c9e9fa5a9 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323) * a7ec07f13 [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324) * 61e95bacb Remove doc reference to the compute method (flutter/engine#38246) * 353d6949e make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325) * 47a358c5e Started using FlutterEngineGroups by default on Android (flutter/engine#37822) * 7d8e10652 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210) * 8915b81d4 Update buildroot to b2ab6e1. (flutter/engine#38329) * 4fe620643 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter#38320)" (flutter/engine#38331) * 2ff490c1e Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333) * 22251857f Add missing include to FlutterThreadSynchronizer (flutter/engine#38337) * 467cfd7ef Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339) * 010f4ee7a Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340) * 773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#38327) * 70439f606 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341) * bc1647f0d Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342) * cac228aff Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344) * 13ae6eb75 Revert "Started using FlutterEngineGroups by default on Android (flutter#37822)" (flutter/engine#38351) * ed8063861 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352) * dcafebf44 Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343) * 6f6158580 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348) * 0c00bc0a9 Remove 30fps cap from playgrounds (flutter/engine#38347) * 38340bb57 [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346) * 3a6b3f986 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357) * 81b453535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358) * d91e20879 Port touch-based tests from embedder integration test (flutter/engine#38234)

flutter-dashboard bot added platform-ios platform-macos labels Dec 15, 2022

Sped up reading with FlutterStandardCodec.

54407e1

gaaclarke force-pushed the faster-codec-read branch from 53d687a to 54407e1 Compare December 15, 2022 21:52

gaaclarke marked this pull request as ready for review December 15, 2022 21:54

gaaclarke requested a review from cyanglaz December 15, 2022 21:54

flutter-dashboard bot added the needs tests label Dec 15, 2022

cyanglaz approved these changes Dec 15, 2022

View reviewed changes

gaaclarke added 4 commits December 15, 2022 14:47

added missing license diff

e47a555

put the IsStandardType in the header so it's closer to where it shoul…

cf38db3

…d change

added unittest for subclassing codecs

dad8623

fixed lints

175c167

gaaclarke added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Dec 16, 2022

auto-submit bot merged commit 773b435 into flutter:main Dec 16, 2022

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 773b4357108e (17 revisions) flutter/flutter#117194

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

e1739d9

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 70439f606bbe (18 revisions) flutter/flutter#117197

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

98ee9e1

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to bc1647f0d2b2 (19 revisions) flutter/flutter#117199

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

99630b8

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to bc1647f0d2b2 (19 revisions) flutter/flutter#117204

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

301cfe6

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to bc1647f0d2b2 (19 revisions) flutter/flutter#117205

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

ac33fc6

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to ed806386167c (22 revisions) flutter/flutter#117218

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

bbea27e

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to dcafebf4445c (23 revisions) flutter/flutter#117219

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

7f2718e

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 6f6158580a1f (24 revisions) flutter/flutter#117223

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

73e8814

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 38340bb5734a (26 revisions) flutter/flutter#117225

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

1b19736

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 3a6b3f9869c4 (27 revisions) flutter/flutter#117230

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

e3614e1

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to 81b453535eb8 (28 revisions) flutter/flutter#117232

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

cb82c46

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to d91e20879a67 (29 revisions) flutter/flutter#117236

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

24b7a04

…38327)

engine-flutter-autoroll mentioned this pull request Dec 16, 2022

Roll Flutter Engine from 29196519c124 to d91e20879a67 (29 revisions) flutter/flutter#117242

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 16, 2022

773b43571 Sped up reading with FlutterStandardCodec. (flutter/engine#…

8a36ec7

…38327)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sped up reading with FlutterStandardCodec. #38327

Sped up reading with FlutterStandardCodec. #38327

gaaclarke commented Dec 15, 2022 •

edited

Loading

flutter-dashboard bot commented Dec 15, 2022

cyanglaz left a comment

cyanglaz Dec 15, 2022

cyanglaz Dec 15, 2022

gaaclarke Dec 15, 2022

cyanglaz Dec 15, 2022

gaaclarke Dec 15, 2022

Sped up reading with FlutterStandardCodec. #38327

Sped up reading with FlutterStandardCodec. #38327

Conversation

gaaclarke commented Dec 15, 2022 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Dec 15, 2022

cyanglaz left a comment

Choose a reason for hiding this comment

cyanglaz Dec 15, 2022

Choose a reason for hiding this comment

cyanglaz Dec 15, 2022

Choose a reason for hiding this comment

gaaclarke Dec 15, 2022

Choose a reason for hiding this comment

cyanglaz Dec 15, 2022

Choose a reason for hiding this comment

gaaclarke Dec 15, 2022

Choose a reason for hiding this comment

gaaclarke commented Dec 15, 2022 •

edited

Loading