Details
Details
- Reviewers
ashoat atul - Commits
- rCOMM038aad4a2924: [lib] Make useAncestorThreads more efficient
Tested three things here:
- I made sure my change to calculating ancestorThreads from ancestorThreadInfos was a no-op:
const ancestorThreads1 = useSelector(state =>
ancestorThreadInfos(threadInfo.id)(state),
);
const ancestorThreads2 = useSelector(ancestorThreadInfos(threadInfo.id));
console.log(_isEqual(ancestorThreads, ancestorThreads1));- I compared the result arrays returned from the original code and the new code:
function useAncestorThreads(
threadInfo: ThreadInfo,
): $ReadOnlyArray<ThreadInfo> {
// This is the original code on `master`
const currentResult = useSelector(state => {
if (!threadIsPending(threadInfo.id)) {
const ancestorThreads = ancestorThreadInfos(threadInfo.id)(state);
if (ancestorThreads.length > 1) {
return ancestorThreads.slice(0, -1);
}
return ancestorThreads;
}
const genesisThreadInfo = threadInfoSelector(state)[genesis.id];
return genesisThreadInfo ? [genesisThreadInfo] : [];
});
// This is the new code provided in this diff
const ancestorThreads1 = useSelector(ancestorThreadInfos(threadInfo.id));
const genesisThreadInfo1 = useSelector(
state => threadInfoSelector(state)[genesis.id],
);
const newResult = React.useMemo(() => {
if (!threadIsPending(threadInfo.id)) {
return ancestorThreads1.length > 1
? ancestorThreads1.slice(0, -1)
: ancestorThreads1;
}
return genesisThreadInfo1 ? [genesisThreadInfo1] : [];
}, [ancestorThreads1, genesisThreadInfo1, threadInfo.id]);
console.log('current vs. new result: ', _isEqual(currentResult, newResult));
return newResult;
}- I went through the app and made sure I didn't notice any regressions from this. I also put log statements outside and inside the useMemo and made sure when unrelated parts of Redux change, only the outside log statement is run again
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/shared/ancestor-threads.js | ||
|---|---|---|
| 17 ↗ | (On Diff #35021) | This is how we do it in thread-ancestors.react.js, and when I did step 1 in my test plan, I didn't notice a difference in the resulting ancestorThreads. Please feel free to let me know if I missed something obvious though and I can change this back to const ancestorThreads = useSelector(state => ancestorThreadInfos(threadInfo.id)(state)); |
Comment Actions
I think this is an improvement. In the past we'd end up generating a new array frequently (see the slice call). Now we'll hopefully "memoize" in those cases and avoid the array regeneration.
Also curious for @atul's take here, since he has a lot of context on React performance.