Improve UI responsiveness when doing node search #15773

pinzart90 · 2025-01-22T14:41:22Z

Purpose

Run the canvas search in a separate thread to improve UI responsiveness.
This PR was created as an alternative to #15733

Summary of changes:
Moved the Lucene IndexSearcher initialization. Previously it was recreated on every search. Now it is created only after the index is modified.
Run in canvas search in a separate thread (thread pool)
Introduced a cancellation token throughout the search operations
New searches will cancel old ones and only update the UI if the search was successful(not cancelled)
These changes will be under a new feature flag

Left to do:
Add tests
Add to library view search and others (pm search, autocomplete )?

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

pinzart90 · 2025-01-22T14:44:30Z

src/DynamoCore/Utilities/LuceneSearchUtility.cs

+            dirReader = writer != null ? writer.GetReader(applyAllDeletes: true) : DirectoryReader.Open(indexDir);
+            Searcher = new(dirReader);
+        }
+


@RobertGlobant20
I opted to initialize the Searcher (and dirReader) after every index modification that I could find.
Do you think I got them all ?

yes, I think all of them were covered.

github-actions · 2025-01-22T15:21:27Z

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

RobertGlobant20

LGTM with one comment

RobertGlobant20 · 2025-01-22T17:58:25Z

src/DynamoCore/Utilities/LuceneSearchUtility.cs

@@ -627,6 +641,7 @@ internal void AddNodeTypeToSearchIndex(NodeSearchElement node, Document doc)
            SetDocumentFieldValue(doc, nameof(LuceneConfig.NodeFieldsEnum.Parameters), node.Parameters ?? string.Empty);

            writer?.AddDocument(doc);
+            InitializeIndexSearcher();


Not sure if this specific call is needed due that usually after a node info is indexed (or all the nodes info were indexed ) we call CommitWriterChanges() - and this call is already calling the InitializeIndexSearcher() method.

RobertGlobant20 · 2025-01-22T18:37:18Z

src/DynamoCore/Utilities/LuceneSearchUtility.cs

+            dirReader = writer != null ? writer.GetReader(applyAllDeletes: true) : DirectoryReader.Open(indexDir);
+            Searcher = new(dirReader);
+        }
+


yes, I think all of them were covered.

RobertGlobant20 · 2025-01-22T18:40:29Z

Coudl you add a GIF showing the behavior of InCanvas Search and Library Search?

mjkkirschner · 2025-01-22T22:44:19Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

        {
            if (DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_debounce", false) ?? false)
            {
                searchDebouncer ??= new ActionDebouncer(dynamoViewModel?.Model?.Logger);
            }
+
+            if (DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_spearate_thread", false) ?? false)


does this exist?

I have to create it. THanks !

spelling separate

mjkkirschner · 2025-01-22T22:45:44Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

@@ -78,6 +80,10 @@ public bool BrowserVisibility
        internal int searchDelayTimeout = 150;
        // Feature flags activated debouncer for the search UI.
        internal ActionDebouncer searchDebouncer = null;
+        // Cancel token source used for the node search operations.
+        internal CancellationTokenSource searchCancelTooken;


spelling token

mjkkirschner · 2025-01-22T22:47:04Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

+                }, searchCancelTooken.Token).ContinueWith((t, o) =>
+                {
+                    // This continuation will execute on the UI thread (forced by using FromCurrentSynchronizationContext())
+                    searchResults = new List<NodeSearchElementViewModel>(t.Result);


so, there should never be concurrent access to this collection because we always cancel when a search is performed?

oh, and it's always on the UI thread as you commented... duh.

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

pinzart90 · 2025-01-22T22:50:05Z

src/DynamoCore/Utilities/LuceneSearchUtility.cs

+        {
+            dirReader = writer != null ? writer.GetReader(applyAllDeletes: true) : DirectoryReader.Open(indexDir);
+            Searcher = new(dirReader);
+        }


not sure if I should lock this, expecting concurrent access to Searcher
All the indexing changes will probably not happen while a user is actively searching ...

mjkkirschner · 2025-01-22T22:51:12Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

@@ -972,11 +1004,11 @@ private void IsSelectedChanged(object sender, PropertyChangedEventArgs e)
        /// </summary>
        /// <returns> Returns a list with a maximum MaxNumSearchResults elements.</returns>
        /// <param name="search"> The search query </param>
-        internal IEnumerable<NodeSearchElementViewModel> Search(string search)
+        internal IEnumerable<NodeSearchElementViewModel> Search(string search, CancellationToken ctk = default)


can you update the params?

mjkkirschner · 2025-01-22T22:52:31Z

src/DynamoCore/Search/NodeSearchModel.cs

        {
-
+            ctk.ThrowIfCancellationRequested();


where do all these get caught?

mjkkirschner · 2025-01-22T23:02:04Z

this seems like a pretty good change, not too hard to reason about.
Any idea if concurrent tests are going to cause issues, I remember some kind of past problem with locked index files.

Do you know if Lucene is itself spinning up new threads or using the thread pool, or touts itself as thread safe, or at least not requiring the main thread?

Also, it just makes me think there's probably something interesting we can do with anticipating searches and performing them in parallel using autocomplete info in the future.

aparajit-pratap · 2025-01-23T03:02:11Z

src/DynamoCore/Search/NodeSearchModel.cs

        {
            var result = Entries.Where(e => {
                if (e.Name.Replace(" ", string.Empty).Equals(nodeName) && e.FullCategoryName.Equals(nodeCategory))
                {
+                    ctk.ThrowIfCancellationRequested();


Where is this caught? Why can't the throw be at the start of the FindModel function?

aparajit-pratap · 2025-01-23T03:07:11Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

+
+            if (DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_spearate_thread", false) ?? false)
+            {
+                enableSeachThreading = true;


enableSearchThreading

aparajit-pratap · 2025-01-23T03:10:39Z

src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs

@@ -87,7 +87,9 @@ internal FeatureFlagsClient(string userkey, string mobileKey = null, bool testMo
                    //in tests we want instancing on so we can test it.
                    { "graphics-primitive-instancing", LdValue.Of(true) },
                    //in tests we want search debouncing on so we can test it.
-                    { "searchbar_debounce", LdValue.Of(true) } });
+                    { "searchbar_debounce", LdValue.Of(true) },


Do we still need this?

pinzart90 added 3 commits January 21, 2025 13:47

update

ba85860

update

37ba109

update

f731c5f

pinzart90 requested review from RobertGlobant20, mjkkirschner and QilongTang January 22, 2025 14:41

pinzart90 commented Jan 22, 2025

View reviewed changes

RobertGlobant20 approved these changes Jan 22, 2025

View reviewed changes

mjkkirschner reviewed Jan 22, 2025

View reviewed changes

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs Show resolved Hide resolved

pinzart90 commented Jan 22, 2025

View reviewed changes

mjkkirschner reviewed Jan 22, 2025

View reviewed changes

src/DynamoCore/Search/NodeSearchModel.cs

{

ctk.ThrowIfCancellationRequested();

Copy link

Member

mjkkirschner Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do all these get caught?

mjkkirschner approved these changes Jan 22, 2025

View reviewed changes

aparajit-pratap reviewed Jan 23, 2025

View reviewed changes

mjkkirschner added the performance label Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UI responsiveness when doing node search #15773

Improve UI responsiveness when doing node search #15773

pinzart90 commented Jan 22, 2025 •

edited

Loading

pinzart90 Jan 22, 2025

RobertGlobant20 Jan 22, 2025

github-actions bot commented Jan 22, 2025

RobertGlobant20 left a comment

RobertGlobant20 Jan 22, 2025

RobertGlobant20 Jan 22, 2025

RobertGlobant20 commented Jan 22, 2025

mjkkirschner Jan 22, 2025

pinzart90 Jan 22, 2025

mjkkirschner Jan 22, 2025

mjkkirschner Jan 22, 2025

mjkkirschner Jan 22, 2025 •

edited

Loading

pinzart90 Jan 22, 2025

mjkkirschner Jan 22, 2025

mjkkirschner Jan 22, 2025

mjkkirschner commented Jan 22, 2025

aparajit-pratap Jan 23, 2025

aparajit-pratap Jan 23, 2025

aparajit-pratap Jan 23, 2025

Improve UI responsiveness when doing node search #15773

Are you sure you want to change the base?

Improve UI responsiveness when doing node search #15773

Conversation

pinzart90 commented Jan 22, 2025 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 22, 2025

UI Smoke Tests

RobertGlobant20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertGlobant20 commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 commented Jan 22, 2025 •

edited

Loading

mjkkirschner Jan 22, 2025 •

edited

Loading