CPLAT-9720 Connect perf optimizations by greglittlefield-wf · Pull Request #462 · Workiva/over_react

greglittlefield-wf · 2020-02-17T22:38:42Z

Motivation

The current implementations of connect / connectFlux by default call into Dart code for almost all of the supported callbacks, and perform shallow operator== equality checks whereas the JS defaults to ===, which is equivalent to Dart's identical operator.

There's not really a good reason for this default, and aligning it to be closer with the JS behavior would reduce performance hits from:

Calling into Dart code
Wrapping JS maps in JsBackedMaps and typed props classes
Calling potentially inefficient operator== implementations on prop values and state objects

Changes

Instead of implementing defaults for these callbacks with Dart wrappers, use the JS defaults for:

(changed from Dart operator== to JS ===)
- areStatesEqual
(changed from shallow props map equality using Dart operator== for values, to shallow props map equality using JS ===)
- areOwnPropsEqual
- areStatePropsEqual
- areMergedPropsEqual

Other changes I made to aid in debugging test failures:

Eliminate most store global variables
Update some test expectations to have better failures (mostly around callback calls)
Remote expect calls from lifecycle callbacks, since they'd fail silently due to not being run in the test zone

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

aviary-wf · 2020-02-17T22:39:56Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

lib/src/over_react_redux/over_react_flux.dart

lib/src/over_react_redux/over_react_redux.dart

joebingham-wk · 2020-02-18T18:49:34Z

test/over_react_redux/connect_flux_test.dart

-          const updateMethodCalls = [
-            'mapStateToProps',
-            'areStatePropsEqual',
-            'mapStateToProps',
-            'areStatePropsEqual'
-          ];


It's dope that the update cycle of connectFlux is more efficient now! However, now connectFlux components and connect components (with areStatesEqual set to false) don't behave the same way. Is that expected?

Good question. I meant to ask you: why, in this case, did we previously expect double the calls?

However, now connectFlux components and connect components (with areStatesEqual set to false) don't behave the same way.

I thought I understood what you were talking about at first but now I'm having trouble wrapping my head around it haha. Can we talk through this offline?

Also just following up on this - if it hasn't been a priority yet and you haven't had a chance to dig in, I'd be happy to as well, just curious if you already did any discovery there :)

Yeah I have no idea haha

Omg I finally figured it out; it's because of the memoizedWrapInteropValue.

In Redux, store.state is updated, resulting in a new object every time, which busts some internal memoization caches, causing extra calls. In Flux, store.state is always the same object.

See more info here: c58c3dc

Niiice, that does make sense! Thanks for following up on that!!

test/over_react_redux/connect_test.dart

…es are identical

joebingham-wk

Just made another pass at the new comments - they all looked good! Just bumping a couple original points

lib/src/over_react_redux/over_react_redux.dart

joebingham-wk · 2020-02-26T17:50:24Z

test/over_react_redux/connect_flux_test.dart

-          const updateMethodCalls = [
-            'mapStateToProps',
-            'areStatePropsEqual',
-            'mapStateToProps',
-            'areStatePropsEqual'
-          ];


Also just following up on this - if it hasn't been a priority yet and you haven't had a chance to dig in, I'd be happy to as well, just curious if you already did any discovery there :)

greglittlefield-wf · 2020-06-08T23:20:50Z

I can't believe I forgot about this... @joebingham-wk would you mind making another pass when you have a sec?

joebingham-wk · 2020-06-09T15:25:07Z

Yee @greglittlefield-wf! There's some stuff on my plate this morning but I'll definitely make another pass today

greglittlefield-wf · 2020-06-09T15:27:43Z

Thanks! No rush btw. This PR has been hanging out for four months; it can wait a couple days 😉 .

greglittlefield-wf · 2020-06-12T16:47:13Z

lib/src/over_react_redux/over_react_flux.dart

-    const propHasher = CollectionLengthHasher();
-    bool areStatePropsEqualWrapper(TProps nextProps, TProps prevProps) {
-      final result = defaultAreStatePropsEqual(nextProps, prevProps);
+  // In dev mode, if areStatePropsEqual is not specified, pass in a version


This block of changes are best reviewed with whitespace-agnostic diff

joebingham-wk

Looks great to me! It'll be awesome to have that memoization in there. I had one tiny real question and then another just curiosity question.

I looked at the failing test - it's a ReduxMultiProvider test that passes locally. I'm wondering if I didn't await correctly and the browser events aren't happening fast enough? Regardless, they do pass locally so it definitely seems like a lame timing thing and not actually related to your changes

lib/src/over_react_redux/over_react_flux.dart

lib/src/over_react_redux/over_react_redux.dart

This reverts commit 64956b0.

greglittlefield-wf · 2020-07-15T00:42:58Z

test/over_react_redux/connect_flux_test.dart

-            expect(methodsCalled.map((methodObj) => methodObj['called']),
-                updateMethodCalls);
+            expect(
+                methodsCalled,
+                expectedUpdateMethodCalls
+                    .map((expected) => containsPair('called', expected))
+                    .toList());


This pattern results in the whole calls being included in the test failure messages, which provides a lot of helpful contextual info.

Before:

Expected: ['mapStateToProps', 'areStatePropsEqual'] Actual: ['mapStateToProps', 'areStatePropsEqual', 'mapStateToProps', 'areStatePropsEqual']

With these changes:

Expected: [ <contains pair 'called' => 'mapStateToProps'>, <contains pair 'called' => 'areStatePropsEqual'> ] Actual: [ { 'called': 'mapStateToProps', 'state': CounterState:CounterState:{count: 1, name: Counter} }, { 'called': 'areStatePropsEqual', 'prev': {'ConnectFluxCounterProps.currentCount': 0}, 'next': {'ConnectFluxCounterProps.currentCount': 1} }, { 'called': 'mapStateToProps', 'state': CounterState:CounterState:{count: 1, name: Counter} }, { 'called': 'areStatePropsEqual', 'prev': {'ConnectFluxCounterProps.currentCount': 1}, 'next': {'ConnectFluxCounterProps.currentCount': 1} } ]

greglittlefield-wf · 2020-07-15T00:43:42Z

test/over_react_redux/connect_flux_test.dart

        },
      };

-      testParameterCases(testCases);


Inlined this so it could access closure variables; it was only being called on this line, anyways.

aaronlademann-wf

Mostly minor stuff. This is gonna be awesome!

aaronlademann-wf · 2020-07-15T14:55:04Z

lib/src/component_declaration/component_base.dart

+      // Use a const list so that empty children prop values are always identical
+      // in the JS props, resulting in JS libraries (e.g., react-redux) and Dart code alike
+      // not marking props as having changed as a result of rerendering the ReactElement with a new list.
+      childArguments = const [];


test/over_react/util/equality_test.dart

test/over_react_redux/connect_flux_test.dart

lib/src/util/equality.dart

joebingham-wk

Looks great to me! +1

aaronlademann-wf

+1

joebingham-wk · 2020-07-15T20:36:53Z

Dope test coverage
Consumer tests pass

QA +1

joebingham-wk · 2020-07-15T21:37:21Z

@Workiva/release-management-p

rmconsole-wf

+1 from RM

rmconsole-wf added the Merge Requirements Unmet label Feb 17, 2020

greglittlefield-wf mentioned this pull request Feb 17, 2020

connect creates new Props map object on every Action dispatch, which must be deeply compared #434

Open

greglittlefield-wf force-pushed the connect-perf branch from 8f06a81 to 34c37ce Compare February 17, 2020 23:04

greglittlefield-wf marked this pull request as ready for review February 18, 2020 00:02

rmconsole2-wf changed the title ~~Connect perf optimizations~~ CPLAT-9720 Connect perf optimizations Feb 18, 2020

davidmarne-wf reviewed Feb 18, 2020

View reviewed changes

lib/src/over_react_redux/over_react_flux.dart Outdated Show resolved Hide resolved

joebingham-wk reviewed Feb 18, 2020

View reviewed changes

rmconsole-wf added the Merge Conflicts label Feb 21, 2020

rmconsole5-wk closed this Feb 24, 2020

greglittlefield-wf added 5 commits February 25, 2020 08:58

Don't call into Dart unnecessarily for equality checks

782b1f5

Fix JS NPEs when passing null functions into connect options

c0be4ae

Add test for memoization of wrapInteropValue and identical states

d47d08e

Fix failing test that expected one extra dispatch for some reason

d0beec3

Use const [] for 0-arg prop children so that JS props.children valu…

ca2ad9c

…es are identical

greglittlefield-wf reopened this Feb 25, 2020

greglittlefield-wf changed the base branch from CPLAT-8693-connect-for-w_flux to master February 25, 2020 16:11

greglittlefield-wf force-pushed the connect-perf branch from 1e63775 to ca2ad9c Compare February 25, 2020 16:11

rmconsole-wf removed the Merge Conflicts label Feb 25, 2020

joebingham-wk reviewed Feb 26, 2020

View reviewed changes

greglittlefield-wf added 4 commits June 8, 2020 15:38

Merge remote-tracking branch 'origin/master' into connect-perf

e0acaa0

Optimize/sort imports

a0dc668

Update doc comments as per CR feedback

d0f5d61

Only wrap areStatePropsEqual when in dev mode

270432f

greglittlefield-wf commented Jun 12, 2020

View reviewed changes

joebingham-wk reviewed Jun 12, 2020

View reviewed changes

lib/src/over_react_redux/over_react_flux.dart Outdated Show resolved Hide resolved

lib/src/over_react_redux/over_react_redux.dart Outdated Show resolved Hide resolved

aaronlademann-wf added the area-redux label Jun 25, 2020

greglittlefield-wf added 14 commits July 2, 2020 16:03

Eliminate global/shared test state

d9f83b8

Fix last remaining global store reference

7c3cef1

Run equality_test as part of browser suites, add missing @teston()

a1b0216

Remove expected duplicate calls

bdec431

Revert inlining of updateMethodCalls, improve failure message

fca7c51

Remove global redux stores in tests

70a449e

Clean up flux store to match redux store

737fe22

Fix expect inside of callback

74790ba

Fix other expects in callbacks

9026660

Update naming, clean up

f797f58

Remove unnecessary test

64956b0

Revert "Remove unnecessary test"

3f9a9bd

This reverts commit 64956b0.

Improve test setup and error messages

1adf774

Update test setup to compare rotten apples to rotten apples

c58c3dc

greglittlefield-wf commented Jul 15, 2020

View reviewed changes

aaronlademann-wf reviewed Jul 15, 2020

View reviewed changes

Address CR feedback

93fb99c

greglittlefield-wf force-pushed the connect-perf branch from 1103dbd to 93fb99c Compare July 15, 2020 16:41

joebingham-wk approved these changes Jul 15, 2020

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Jul 15, 2020

aaronlademann-wf approved these changes Jul 15, 2020

View reviewed changes

rmconsole-wf added the RM Ready label Jul 15, 2020

rmconsole-wf approved these changes Jul 15, 2020

View reviewed changes

rmconsole6-wk merged commit edc2de5 into master Jul 15, 2020

rmconsole6-wk deleted the connect-perf branch July 15, 2020 21:45

rmconsole-readonly-wk mentioned this pull request Jul 17, 2020

RM-78027 RM-78026 Release over_react 3.7.0 #609

Merged

Conversation

greglittlefield-wf commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

Uh oh!

aviary-wf commented Feb 17, 2020

Security Insights

Action Items

Uh oh!

Uh oh!

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!

Uh oh!

joebingham-wk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglittlefield-wf commented Jun 8, 2020

Uh oh!

joebingham-wk commented Jun 9, 2020

Uh oh!

greglittlefield-wf commented Jun 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joebingham-wk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joebingham-wk left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Uh oh!

joebingham-wk commented Jul 15, 2020

Uh oh!

joebingham-wk commented Jul 15, 2020

Uh oh!

rmconsole-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

greglittlefield-wf commented Feb 17, 2020 •

edited

Loading