Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve UI responsiveness when doing node search #15773

Merged
merged 10 commits into from
Feb 3, 2025
40 changes: 31 additions & 9 deletions src/DynamoCore/Search/NodeSearchModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading;
using System.Xml;
using Dynamo.Configuration;
using Dynamo.Graph.Nodes;
Expand All @@ -10,7 +11,6 @@
using Dynamo.Utilities;
using DynamoUtilities;
using Lucene.Net.Documents;
using Lucene.Net.Index;
using Lucene.Net.QueryParsers.Classic;
using Lucene.Net.Search;

Expand Down Expand Up @@ -231,29 +231,42 @@ internal string ProcessNodeCategory(string category, ref SearchElementGroup grou
return category.Substring(0, index);
}

internal IEnumerable<NodeSearchElement> Search(string search, LuceneSearchUtility luceneSearchUtility)
/// <summary>
/// Search for nodes by using a search key.
/// </summary>
/// <param name="search"></param>
/// <param name="luceneSearchUtility"></param>
/// <param name="ctk">Cancellation token to short circuit the search.</param>
/// <returns></returns>
/// <exception cref="Exception"></exception>
internal IEnumerable<NodeSearchElement> Search(string search, LuceneSearchUtility luceneSearchUtility, CancellationToken ctk = default)
{

ctk.ThrowIfCancellationRequested();
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These get caught by the Task object that wraps the parent caller

Task.Run(() =>
{
   return Search(query, searchCancelToken.Token);
}


if (luceneSearchUtility != null)
{
//The DirectoryReader and IndexSearcher have to be assigned after commiting indexing changes and before executing the Searcher.Search() method, otherwise new indexed info won't be reflected
luceneSearchUtility.dirReader = luceneSearchUtility.writer != null ? luceneSearchUtility.writer.GetReader(applyAllDeletes: true) : DirectoryReader.Open(luceneSearchUtility.indexDir);
luceneSearchUtility.Searcher = new IndexSearcher(luceneSearchUtility.dirReader);
if (luceneSearchUtility.Searcher == null)
{
throw new Exception("Invalid IndexSearcher found");
}

string searchTerm = search.Trim();
var candidates = new List<NodeSearchElement>();

var parser = new MultiFieldQueryParser(LuceneConfig.LuceneNetVersion, LuceneConfig.NodeIndexFields, luceneSearchUtility.Analyzer)
{
AllowLeadingWildcard = true,
DefaultOperator = LuceneConfig.DefaultOperator,
FuzzyMinSim = LuceneConfig.MinimumSimilarity
};

Query query = parser.Parse(luceneSearchUtility.CreateSearchQuery(LuceneConfig.NodeIndexFields, searchTerm));
Query query = parser.Parse(luceneSearchUtility.CreateSearchQuery(LuceneConfig.NodeIndexFields, searchTerm, false, ctk));
TopDocs topDocs = luceneSearchUtility.Searcher.Search(query, n: LuceneConfig.DefaultResultsCount);

for (int i = 0; i < topDocs.ScoreDocs.Length; i++)
{
ctk.ThrowIfCancellationRequested();

// read back a Lucene doc from results
Document resultDoc = luceneSearchUtility.Searcher.Doc(topDocs.ScoreDocs[i].Doc);

Expand All @@ -268,7 +281,7 @@ internal IEnumerable<NodeSearchElement> Search(string search, LuceneSearchUtilit
}
else
{
var foundNode = FindModelForNodeNameAndCategory(name, cat, parameters);
var foundNode = FindModelForNodeNameAndCategory(name, cat, parameters, ctk);
if (foundNode != null)
{
candidates.Add(foundNode);
Expand All @@ -280,11 +293,20 @@ internal IEnumerable<NodeSearchElement> Search(string search, LuceneSearchUtilit
return null;
}

internal NodeSearchElement FindModelForNodeNameAndCategory(string nodeName, string nodeCategory, string parameters)
/// <summary>
/// Finds the node model that corresponds to the input nodeName, nodeCategory and parameters.
/// </summary>
/// <param name="nodeName"></param>
/// <param name="nodeCategory"></param>
/// <param name="parameters"></param>
/// <param name="ctk">Cancellation token to short circuit the operation.</param>
/// <returns></returns>
internal NodeSearchElement FindModelForNodeNameAndCategory(string nodeName, string nodeCategory, string parameters, CancellationToken ctk = default)
{
var result = Entries.Where(e => {
if (e.Name.Replace(" ", string.Empty).Equals(nodeName) && e.FullCategoryName.Equals(nodeCategory))
{
ctk.ThrowIfCancellationRequested();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to short circuit the Where operation as well.
I will actually move it above the if, just to make the operation finish faster in case the if condition is not met

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will put on at the beginning too

//When the node info was indexed if Parameters was null we added an empty space (null cannot be indexed)
//Then in this case when searching if e.Parameters is null we need to check against empty space
if (e.Parameters == null)
Expand Down
27 changes: 21 additions & 6 deletions src/DynamoCore/Utilities/LuceneSearchUtility.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading;
using Dynamo.Configuration;
using Dynamo.Models;
using Dynamo.Search.SearchElements;
Expand All @@ -23,7 +24,6 @@
using Lucene.Net.Search;
using Lucene.Net.Store;
using Lucene.Net.Util;
using Newtonsoft.Json;

namespace Dynamo.Utilities
{
Expand Down Expand Up @@ -190,6 +190,12 @@ internal void CreateLuceneIndexWriter(OpenMode mode = OpenMode.CREATE)
}
}

private void InitializeIndexSearcher()
{
dirReader = writer != null ? writer.GetReader(applyAllDeletes: true) : DirectoryReader.Open(indexDir);
Searcher = new(dirReader);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I think all of them were covered.

/// <summary>
/// Initialize Lucene index document object for reuse
/// </summary>
Expand Down Expand Up @@ -256,7 +262,8 @@ internal void UpdateIndexedNodesInfo(List<NodeSearchElement> nodeList)
{
var iDoc = InitializeIndexDocumentForNodes();
AddNodeTypeToSearchIndex(node, iDoc);
}
}
InitializeIndexSearcher();
}
}

Expand Down Expand Up @@ -321,8 +328,9 @@ internal void SetDocumentFieldValue(Document doc, string field, string value, bo
/// <param name="fields">All fields to be searched in.</param>
/// <param name="SearchTerm">Search key to be searched for.</param>
/// <param name="IsPackageContext">Set this to true if the search context is packages instead of nodes.</param>
/// <param name="ctk">Cancellation token to short circuit the search.</param>
/// <returns></returns>
internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPackageContext = false)
internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPackageContext = false, CancellationToken ctk = default)
{
//By Default the search will be normal
SearchType searchType = SearchType.Normal;
Expand Down Expand Up @@ -353,6 +361,7 @@ internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPac
foreach (string f in fields)
{
Occur occurQuery = Occur.SHOULD;
ctk.ThrowIfCancellationRequested();

searchTerm = QueryParser.Escape(SearchTerm);
if (searchType == SearchType.ByDotCategory)
Expand Down Expand Up @@ -383,7 +392,7 @@ internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPac
continue;

//Adds the FuzzyQuery and 4 WildcardQueries (3 of them contain regular expressions), with the normal weights
AddQueries(searchTerm, f, searchType, booleanQuery, occurQuery, fuzzyLogicMaxEdits);
AddQueries(searchTerm, f, searchType, booleanQuery, occurQuery, fuzzyLogicMaxEdits, ctk);

if (searchType == SearchType.ByEmptySpace)
{
Expand All @@ -396,7 +405,7 @@ internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPac
if (string.IsNullOrEmpty(s)) continue;

//Adds the FuzzyQuery and 4 WildcardQueries (3 of them contain regular expressions), with the weights for Queries with RegularExpressions
AddQueries(s, f, searchType, booleanQuery, occurQuery, LuceneConfig.FuzzySearchMinEdits, true);
AddQueries(s, f, searchType, booleanQuery, occurQuery, LuceneConfig.FuzzySearchMinEdits, ctk, true);
}
}
}
Expand All @@ -412,9 +421,12 @@ internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPac
/// <param name="booleanQuery">The Boolean query in which the Wildcard queries will be added</param>
/// <param name="occurQuery">Occur type can be Should or Must</param>
/// <param name="fuzzyLogicMaxEdits">Max edit lenght for Fuzzy queries</param>
/// <param name="ctk">Cancellation token to short circuit the operation.</param>
/// <param name="termSplit">Indicates if the SearchTerm has been split by empty space or not</param>
private void AddQueries(string searchTerm, string field, SearchType searchType, BooleanQuery booleanQuery, Occur occurQuery, int fuzzyLogicMaxEdits, bool termSplit = false)
private void AddQueries(string searchTerm, string field, SearchType searchType, BooleanQuery booleanQuery, Occur occurQuery, int fuzzyLogicMaxEdits, CancellationToken ctk = default, bool termSplit = false)
{
ctk.ThrowIfCancellationRequested();

string querySearchTerm = searchTerm.Replace(" ", string.Empty);

FuzzyQuery fuzzyQuery;
Expand Down Expand Up @@ -582,6 +594,8 @@ internal void CommitWriterChanges()
{
//Commit the info indexed if index writer exists
writer?.Commit();

InitializeIndexSearcher();
}

/// <summary>
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

This call is causing Dynamo startup to balloon to more than a minute.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should have a perf test for this

}
}

Expand Down
56 changes: 44 additions & 12 deletions src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Media;
using Dynamo.Configuration;
Expand Down Expand Up @@ -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;
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
// Enable running Search on a thread pool thread.
private bool enableSeachThreading;

private string searchText = string.Empty;
/// <summary>
Expand Down Expand Up @@ -398,17 +404,22 @@ internal SearchViewModel(DynamoViewModel dynamoViewModel)

iconServices = new IconServices(pathManager);

DynamoFeatureFlagsManager.FlagsRetrieved += TryInitializeDebouncer;
DynamoFeatureFlagsManager.FlagsRetrieved += TryInitializeSearchFlags;

InitializeCore();
}

private void TryInitializeDebouncer()
private void TryInitializeSearchFlags()
{
if (DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_debounce", false) ?? false)
{
searchDebouncer ??= new ActionDebouncer(dynamoViewModel?.Model?.Logger);
}

if (DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_spearate_thread", false) ?? false)
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
{
enableSeachThreading = true;
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
}
}

// Just for tests. Please refer to LibraryTests.cs
Expand Down Expand Up @@ -442,7 +453,7 @@ public override void Dispose()
Model.EntryRemoved -= RemoveEntry;

searchDebouncer?.Dispose();
DynamoFeatureFlagsManager.FlagsRetrieved -= TryInitializeDebouncer;
DynamoFeatureFlagsManager.FlagsRetrieved -= TryInitializeSearchFlags;

base.Dispose();
}
Expand Down Expand Up @@ -473,7 +484,7 @@ private void InitializeCore()
InsertClassesIntoTree(LibraryRootCategories);

// If feature flags are already cached, try to initialize the debouncer
TryInitializeDebouncer();
TryInitializeSearchFlags();
}

private void AddEntry(NodeSearchElement entry)
Expand Down Expand Up @@ -924,15 +935,36 @@ public void SearchAndUpdateResults(string query)
if (string.IsNullOrEmpty(query))
return;

//Passing the second parameter as true will search using Lucene.NET
var foundNodes = Search(query);
searchResults = new List<NodeSearchElementViewModel>(foundNodes);
if (enableSeachThreading)
{
// A new search should cancel any existing searches.
searchCancelTooken?.Cancel();
searchCancelTooken?.Dispose();
searchCancelTooken = new();

FilteredResults = searchResults;
// We run the searches on the thread pool to reduce the impact on the UI thread.
Task.Run(() =>
{
return Search(query, searchCancelTooken.Token);

UpdateSearchCategories();
}, searchCancelTooken.Token).ContinueWith((t, o) =>
{
// This continuation will execute on the UI thread (forced by using FromCurrentSynchronizationContext())
searchResults = new List<NodeSearchElementViewModel>(t.Result);
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved

RaisePropertyChanged("FilteredResults");
mjkkirschner marked this conversation as resolved.
Show resolved Hide resolved
FilteredResults = searchResults;
UpdateSearchCategories();
}, TaskScheduler.FromCurrentSynchronizationContext(), TaskContinuationOptions.OnlyOnRanToCompletion);
}
else
{
//Passing the second parameter as true will search using Lucene.NET
var foundNodes = Search(query);
searchResults = new List<NodeSearchElementViewModel>(foundNodes);

FilteredResults = searchResults;
UpdateSearchCategories();
}
}

/// <summary>
Expand Down Expand Up @@ -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)
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
{
if (LuceneUtility != null)
{
var searchElements = Model.Search(search, LuceneUtility);
var searchElements = Model.Search(search, LuceneUtility, ctk);
if (searchElements != null)
{
return searchElements.Select(MakeNodeSearchElementVM);
Expand Down
4 changes: 3 additions & 1 deletion src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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) },
pinzart90 marked this conversation as resolved.
Show resolved Hide resolved
//in tests we want to run search on non UI thread so we can test it.
{ "searchbar_spearate_thread", LdValue.Of(true) }});
return;
}

Expand Down
Loading