Page MenuHomePhabricator

[lib] Make useAncestorThreads more efficient
ClosedPublic

Authored by rohan on Dec 27 2023, 8:10 AM.
Tags
None
Referenced Files
F3505420: D10467.diff
Fri, Dec 20, 1:23 PM
Unknown Object (File)
Fri, Dec 13, 9:48 PM
Unknown Object (File)
Tue, Dec 3, 2:48 AM
Unknown Object (File)
Oct 31 2024, 12:51 AM
Unknown Object (File)
Oct 31 2024, 12:51 AM
Unknown Object (File)
Oct 31 2024, 12:51 AM
Unknown Object (File)
Oct 31 2024, 12:51 AM
Unknown Object (File)
Oct 3 2024, 12:28 PM
Subscribers

Details

Summary

When working on another urgent issue, @ashoat pointed out that useAncestorThreads currently returns a new result array every time Redux changes. It made more sense to extract the relevant parts of the state out, then do the rest of the calculations inside a useMemo.

Addresses ENG-6160

Test Plan

Tested three things here:

  1. 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));
  1. 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;
}
  1. 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

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan edited the test plan for this revision. (Show Details)
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));

rohan edited the test plan for this revision. (Show Details)
rohan requested review of this revision.Dec 27 2023, 8:29 AM
ashoat added a reviewer: atul.
ashoat added a subscriber: atul.

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.

This revision is now accepted and ready to land.Dec 27 2023, 3:21 PM