Page MenuHomePhabricator

[lib] Add chat mention SearchIndex selector
ClosedPublic

Authored by patryk on Aug 22 2023, 2:04 AM.
Tags
None
Referenced Files
F1865024: D8900.id.diff
Sun, May 26, 9:06 AM
Unknown Object (File)
Mar 31 2024, 8:33 AM
Unknown Object (File)
Mar 31 2024, 8:30 AM
Unknown Object (File)
Mar 30 2024, 3:30 PM
Unknown Object (File)
Mar 27 2024, 5:28 PM
Unknown Object (File)
Mar 4 2024, 10:50 AM
Unknown Object (File)
Mar 4 2024, 10:50 AM
Unknown Object (File)
Mar 4 2024, 10:50 AM
Subscribers

Details

Summary

Solution for ENG-4552.

This diff introduces utilities for storing and using calculated SearchIndex for each thread in threadStore.

Depends on D8899.

Test Plan

Tested later in the stack.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

patryk held this revision as a draft.
patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: tomek, inka.
patryk edited the summary of this revision. (Show Details)
inka requested changes to this revision.Aug 29 2023, 4:51 AM

If I understand correctly, this structure is build by creating a SentencePrefixSearchIndex for each community (or subtree with a root being a child of GENESIS), and having a "map" from every chat, to its corresponding SentencePrefixSearchIndex. But I don't understand why we needed to exclude the thread itself from its chatMentionCandidates, but we don't need to exclude it from this structure? Can you explain please?

Also - I think this code will change a lot after D8833 is refactored.

This revision now requires changes to proceed.Aug 29 2023, 4:51 AM

Discussed this offline:

But I don't understand why we needed to exclude the thread itself from its chatMentionCandidates, but we don't need to exclude it from this structure? Can you explain please?

Excluding the thread in which we mention was being handled by getMentionTypeaheadChatSuggestions introduced in D8910 (see for (const threadID in chatMentionCandidates) loop). But this is going to be modified: to avoid unnecessary for loop we can either extract thread itself from useThreadChatMentionSearchIndex or just to add if condition in getMentionTypeaheadChatSuggestions which filters out current thread id.

btw, I think that getMentionTypeaheadChatSuggestions should not be defined in D8910, but in separate diff...

Change useChatMentionSearchIndex

This revision is now accepted and ready to land.Sep 6 2023, 1:07 AM
tomek requested changes to this revision.Sep 12 2023, 8:05 AM
tomek added inline comments.
lib/shared/thread-utils.js
1780–1794 ↗(On Diff #30614)

There are two possible performance issues here:

  1. Every time there's a change in a value returned by useChatMentionCandidatesObjAndUtils we're recomputing all the indexes for all the threads. It would be great if we could somehow figure out which threads should be actually recomputed (by e.g. using memoization).
  2. We compute separate indices for each thread, which is really wasteful because most of the threads can be mentioned from a lot of threads. What we can do instead is to have a single search index, from which we get the ids and then filter what threads can be mentioned from a thread.
1803–1810 ↗(On Diff #30614)

Why do we have to do something special for genesis here?

This revision now requires changes to proceed.Sep 12 2023, 8:05 AM
lib/shared/thread-utils.js
1780–1794 ↗(On Diff #30614)
  1. Right now, this code has a time complexity of O(|no_of_communities + no_of_first_genesis_children| * |threads_in_community|). If we have an enormous thread store, that might be a problem. To change this code's behavior, useChatMentionCandidatesObjAndUtils needs to be refactored. We could add more logic to the current useMemo implementation and use a utility function that stores the previous value (e.g. a trick with useRef and useEffect) and compares objects using JSON.stringify if the thread store changes.
  2. We don't compute separate indices for each thread, threadID in for loop is actually a communityThreadID, it's confusing so renaming it to communityThreadID sounds like a better idea.
1803–1810 ↗(On Diff #30614)

See the argument: communityThreadIDForGenesisThreads[threadInfo.id].

Is the performance concern pointed out by @tomek going to lead to additional React render cycles? If yes, we should prioritize addressing it. cc @atul

Is the performance concern pointed out by @tomek going to lead to additional React render cycles? If yes, we should prioritize addressing it. cc @atul

It could, because every time something changes inside one community, we recompute every search index, instead of just the one that might be affected. But this shouldn't be a big issue - it's only one, rare render. In the solution I proposed, this issue will still be present.

So overall, my ideas were about improving the performance of an algorithm, without reducing the number of renders (significantly).

This revision is now accepted and ready to land.Sep 20 2023, 3:34 AM
This revision is now accepted and ready to land.Sep 28 2023, 5:11 AM