-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CPLAT-9720 Connect perf optimizations #462
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
8f06a81
to
34c37ce
Compare
const updateMethodCalls = [ | ||
'mapStateToProps', | ||
'areStatePropsEqual', | ||
'mapStateToProps', | ||
'areStatePropsEqual' | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I have no idea haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niiice, that does make sense! Thanks for following up on that!!
…es are identical
1e63775
to
ca2ad9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just made another pass at the new comments - they all looked good! Just bumping a couple original points
const updateMethodCalls = [ | ||
'mapStateToProps', | ||
'areStatePropsEqual', | ||
'mapStateToProps', | ||
'areStatePropsEqual' | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
I can't believe I forgot about this... @joebingham-wk would you mind making another pass when you have a sec? |
Yee @greglittlefield-wf! There's some stuff on my plate this morning but I'll definitely make another pass today |
Thanks! No rush btw. This PR has been hanging out for four months; it can wait a couple days 😉 . |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of changes are best reviewed with whitespace-agnostic diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This reverts commit 64956b0.
expect(methodsCalled.map((methodObj) => methodObj['called']), | ||
updateMethodCalls); | ||
expect( | ||
methodsCalled, | ||
expectedUpdateMethodCalls | ||
.map((expected) => containsPair('called', expected)) | ||
.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}
}
]
@@ -691,7 +722,53 @@ main() { | |||
}, | |||
}; | |||
|
|||
testParameterCases(testCases); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inlined this so it could access closure variables; it was only being called on this line, anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor stuff. This is gonna be awesome!
// 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 []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1103dbd
to
93fb99c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
QA +1 |
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
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'sidentical
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:
operator==
implementations on prop values and state objectsChanges
Instead of implementing defaults for these callbacks with Dart wrappers, use the JS defaults for:
operator==
to JS===
)areStatesEqual
operator==
for values, to shallow props map equality using JS===
)areOwnPropsEqual
areStatePropsEqual
areMergedPropsEqual
Other changes I made to aid in debugging test failures:
expect
calls from lifecycle callbacks, since they'd fail silently due to not being run in the test zoneRelease Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: