[lib] Use RadixTree in SearchIndex
ClosedPublic
Actions

Authored by ashoat on Oct 27 2023, 11:32 AM.

Details

Reviewers

tomek
atul
inka
rohan

Commits

rCOMMc3231ddf2158: [lib] Use RadixTree in SearchIndex

Summary

This diff actually uses the RadixTree introduced in the parent diff from the SearchIndex code.

Linear tasks: ENG-5137 and ENG-5480

Depends on D9626

Test Plan

I tested the chat mentions experience
I did some perf testing:

In combination with the previous diff, I used this patch to test performance before and after this change. I made sure I had at least three samples of each scenario. Will also link my messy Gist of results, but it's not really interpretable by anyone other than me.

Here's the relevant portion:

BEFORE

 LOG  useChatMentionSearchIndex took 1801ms
 LOG  useChatMentionSearchIndex took 1748ms
 LOG  useChatMentionSearchIndex took 1730ms
 LOG  useChatMentionSearchIndex took 1831ms

 AVERAGE 1777.5ms

JUST DEDUP (parent diff)

 LOG  useChatMentionSearchIndex took 1027ms
 LOG  useChatMentionSearchIndex took 949ms
 LOG  useChatMentionSearchIndex took 957ms

 AVERAGE 977.7ms

DEDUP + RADIX TREE

 LOG  useChatMentionSearchIndex took 643ms
 LOG  useChatMentionSearchIndex took 629ms
 LOG  useChatMentionSearchIndex took 651ms
 LOG  useChatMentionSearchIndex took 609ms

 AVERAGE 633ms

JUST RADIX TREE

 LOG  useChatMentionSearchIndex took 1394ms
 LOG  useChatMentionSearchIndex took 1468ms
 LOG  useChatMentionSearchIndex took 1511ms
 LOG  useChatMentionSearchIndex took 1492ms
 LOG  useChatMentionSearchIndex took 1397ms

 AVERAGE 1452.4ms

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Oct 27 2023, 11:32 AM

Herald added a subscriber: will. · View Herald TranscriptOct 27 2023, 11:32 AM

Harbormaster completed remote builds in B23551: Diff 32476.Oct 27 2023, 11:56 AM

ashoat requested review of this revision.Oct 27 2023, 11:56 AM

tomek accepted this revision.Oct 30 2023, 4:13 AM

tomek added inline comments.

lib/shared/search-index.js
66 ↗	(On Diff #32476)	Can this be simplified?

This revision is now accepted and ready to land.Oct 30 2023, 4:13 AM

ashoat added inline comments.Oct 30 2023, 11:47 AM

lib/shared/search-index.js
66 ↗	(On Diff #32476)	I tested this approach in the Chrome debug console and found that it didn't work:

ashoat attached a referenced file: F842952: Screenshot 2023-10-30 at 2.47.26 PM.png. (Show Details)Oct 30 2023, 11:48 AM

Closed by commit rCOMMc3231ddf2158: [lib] Use RadixTree in SearchIndex (authored by ashoat). · Explain WhyOct 30 2023, 12:24 PM

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMMc3231ddf2158: [lib] Use RadixTree in SearchIndex.

tomek added inline comments.Oct 31 2023, 1:34 AM

lib/shared/search-index.js
66 ↗	(On Diff #32476)	Ah, that makes sense. In order to make it work, we would need to bind the method which will be more complicated than the current solution.