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.