Skip to content

Commit

Permalink
[lib] Make useAncestorThreads more efficient
Browse files Browse the repository at this point in the history
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 [[ https://linear.app/comm/issue/ENG-6160/make-useancestorthreads-more-efficient | 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));
```

2. 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;
}
```

3. 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

Reviewers: ashoat, atul

Reviewed By: ashoat, atul

Subscribers: atul, tomek, ashoat

Differential Revision: https://phab.comm.dev/D10467
  • Loading branch information
RohanK6 committed Dec 28, 2023
1 parent a767e93 commit 038aad4
Showing 1 changed file with 13 additions and 9 deletions.
22 changes: 13 additions & 9 deletions lib/shared/ancestor-threads.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// @flow

import * as React from 'react';

import genesis from '../facts/genesis.js';
import {
threadInfoSelector,
Expand All @@ -12,18 +14,20 @@ import { useSelector } from '../utils/redux-utils.js';
function useAncestorThreads(
threadInfo: ThreadInfo,
): $ReadOnlyArray<ThreadInfo> {
return useSelector(state => {
if (!threadIsPending(threadInfo.id)) {
const ancestorThreads = ancestorThreadInfos(threadInfo.id)(state);
if (ancestorThreads.length > 1) {
return ancestorThreads.slice(0, -1);
}
const ancestorThreads = useSelector(ancestorThreadInfos(threadInfo.id));

return ancestorThreads;
const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id],
);

return React.useMemo(() => {
if (!threadIsPending(threadInfo.id)) {
return ancestorThreads.length > 1
? ancestorThreads.slice(0, -1)
: ancestorThreads;
}
const genesisThreadInfo = threadInfoSelector(state)[genesis.id];
return genesisThreadInfo ? [genesisThreadInfo] : [];
});
}, [ancestorThreads, genesisThreadInfo, threadInfo.id]);
}

export { useAncestorThreads };

0 comments on commit 038aad4

Please sign in to comment.