Page MenuHomePhabricator
Differential D9627

[lib] Use RadixTree in SearchIndex
ClosedPublic
Actions

Authored by ashoat on Oct 27 2023, 11:32 AM.
Tags
None
Referenced Files
F2899879: D9627.id32476.diff
Sat, Oct 5, 8:29 AM
F2899302: D9627.diff
Sat, Oct 5, 6:26 AM
Unknown Object (File)
Wed, Oct 2, 2:25 PM
Unknown Object (File)
Wed, Oct 2, 2:25 PM
Unknown Object (File)
Wed, Oct 2, 2:25 PM
Unknown Object (File)
Wed, Oct 2, 2:22 PM
Unknown Object (File)
Sun, Sep 29, 5:39 AM
Unknown Object (File)
Sat, Sep 7, 10:48 PM
View All 51 Files
Subscribers
will

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
  1. I tested the chat mentions experience
  2. 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
No Lint Coverage
Unit
No Test Coverage

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

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

I tested this approach in the Chrome debug console and found that it didn't work:

Screenshot 2023-10-30 at 2.47.26 PM.png (592×1 px, 94 KB)

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

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.

Revision Contents
Changeset List

PathSize
lib/
shared/
37 lines
6 lines

Diff 32476

View Options

lib/shared/search-index.js

Loading...
View Options

lib/shared/sentence-prefix-search-index.js

Loading...