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)

)

* e28b26e [linux] Allow overriding asset, ICU data path (flutter/engine#38296) * 35bdb8b Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319) * 97e246c Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320) * d9580a5 Migrate iOS text input plugin to use ARC (flutter/engine#38179) * c9e9fa5 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323) * a7ec07f [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324) * 61e95ba Remove doc reference to the compute method (flutter/engine#38246) * 353d694 make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325) * 47a358c Started using FlutterEngineGroups by default on Android (flutter/engine#37822) * 7d8e106 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210) * 8915b81 Update buildroot to b2ab6e1. (flutter/engine#38329) * 4fe6206 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (#38320)" (flutter/engine#38331) * 2ff490c Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333) * 2225185 Add missing include to FlutterThreadSynchronizer (flutter/engine#38337) * 467cfd7 Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339) * 010f4ee Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340) * 773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327) * 70439f6 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341) * bc1647f Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342) * cac228a Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344) * 13ae6eb Revert "Started using FlutterEngineGroups by default on Android (#37822)" (flutter/engine#38351) * ed80638 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352) * dcafebf Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343) * 6f61585 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348) * 0c00bc0 Remove 30fps cap from playgrounds (flutter/engine#38347) * 38340bb [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346) * 3a6b3f9 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357) * 81b4535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358) * d91e208 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

…17242) * e28b26e [linux] Allow overriding asset, ICU data path (flutter/engine#38296) * 35bdb8b Roll Skia from 9f728d78f10d to f549128104ba (1 revision) (flutter/engine#38319) * 97e246c Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter/engine#38320) * d9580a5 Migrate iOS text input plugin to use ARC (flutter/engine#38179) * c9e9fa5 Update web_sdk -> package test dependency to get updated package matcher (flutter/engine#38323) * a7ec07f [fuchsia] Manually roll Fuchsia Linux SDK. (flutter/engine#38324) * 61e95ba Remove doc reference to the compute method (flutter/engine#38246) * 353d694 make sure CanvasRecorder updates clip bounds methods (flutter/engine#38325) * 47a358c Started using FlutterEngineGroups by default on Android (flutter/engine#37822) * 7d8e106 Bump github/codeql-action from 2.1.35 to 2.1.36 (flutter/engine#38210) * 8915b81 Update buildroot to b2ab6e1. (flutter/engine#38329) * 4fe6206 Revert "Roll Dart SDK from 358d0d1aa3e7 to 1dd5b1bf1099 (7 revisions) (flutter#38320)" (flutter/engine#38331) * 2ff490c Roll Skia from f549128104ba to 5e69caecd166 (11 revisions) (flutter/engine#38333) * 2225185 Add missing include to FlutterThreadSynchronizer (flutter/engine#38337) * 467cfd7 Roll Fuchsia Mac SDK from VEOIaacOA75U7PYyz... to KtItDj-MERuua77aS... (flutter/engine#38339) * 010f4ee Roll Fuchsia Linux SDK from zwfwHRSLdmV61hYqe... to urDNtEiHFAcBBhYe0... (flutter/engine#38340) * 773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327) * 70439f6 Roll Skia from 5e69caecd166 to 62f22c9c7d67 (3 revisions) (flutter/engine#38341) * bc1647f Roll the test package used by Web in preparation for a Dart 3 SDK roll (flutter/engine#38342) * cac228a Roll Dart SDK from 358d0d1aa3e7 to 7b4d4ec3cad1 (14 revisions) (flutter/engine#38344) * 13ae6eb Revert "Started using FlutterEngineGroups by default on Android (flutter#37822)" (flutter/engine#38351) * ed80638 Add an explicit constraint on the matcher package version to ensure Dart 3 compatibility (flutter/engine#38352) * dcafebf Roll Skia from 62f22c9c7d67 to 1b1f53d77ced (1 revision) (flutter/engine#38343) * 6f61585 Roll Fuchsia Mac SDK from KtItDj-MERuua77aS... to bn5VF1-xDf-wKjIw8... (flutter/engine#38348) * 0c00bc0 Remove 30fps cap from playgrounds (flutter/engine#38347) * 38340bb [Impeller] Fix SceneC crash for nodes with children (flutter/engine#38346) * 3a6b3f9 Roll Fuchsia Linux SDK from urDNtEiHFAcBBhYe0... to H6B0UgW07fc1nBtnc... (flutter/engine#38357) * 81b4535 Roll Skia from 1b1f53d77ced to 7b0a9d9a3008 (8 revisions) (flutter/engine#38358) * d91e208 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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

e1739d9

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

98ee9e1

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

99630b8

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

301cfe6

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

ac33fc6

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

bbea27e

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

7f2718e

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

1b19736

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

e3614e1

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

cb82c46

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

24b7a04

)

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

773b435 Sped up reading with FlutterStandardCodec. (flutter/engine#38327

8a36ec7

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sped up reading with FlutterStandardCodec. #38327

Sped up reading with FlutterStandardCodec. #38327

Uh oh!

gaaclarke commented Dec 15, 2022 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Dec 15, 2022

Uh oh!

cyanglaz left a comment

Uh oh!

cyanglaz Dec 15, 2022

Uh oh!

cyanglaz Dec 15, 2022

Uh oh!

gaaclarke Dec 15, 2022

Uh oh!

cyanglaz Dec 15, 2022

Uh oh!

gaaclarke Dec 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sped up reading with FlutterStandardCodec. #38327

Sped up reading with FlutterStandardCodec. #38327

Uh oh!

Conversation

gaaclarke commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Dec 15, 2022

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

cyanglaz Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

cyanglaz Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

gaaclarke Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

cyanglaz Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

gaaclarke Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaaclarke commented Dec 15, 2022 •

edited

Loading