-
Notifications
You must be signed in to change notification settings - Fork 635
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
Node Search performance improvements #12056
Changes from 12 commits
06d0ad3
ec2fd27
4ad7d64
a4a0298
ca08dd7
6344b71
6f1341d
3c4eca2
cc1db45
7124644
ebe06b4
7be230c
ab05505
ce3999d
3216447
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ namespace Dynamo.Search | |
public class SearchDictionary<V> | ||
{ | ||
private ILogger logger; | ||
private static int LIMIT_SEARCH_TAG_SIZE = 300; | ||
|
||
/// <summary> | ||
/// Construct a SearchDictionary object | ||
|
@@ -277,7 +278,8 @@ private static bool MatchWithQueryString(string key, string[] subPatterns) | |
for (int i = subPattern.Length; i >= 1; i--) | ||
{ | ||
var part = subPattern.Substring(0, i); | ||
if (key.IndexOf(part) != -1) | ||
// Use OrdinalIgnoreCase to improve performance (with the accepted downside that the culture will be ignored) | ||
if (key.IndexOf(part, StringComparison.OrdinalIgnoreCase) != -1) | ||
{ //if we find a match record the amount of the match and goto the next word | ||
numberOfMatchSymbols += part.Length; | ||
break; | ||
|
@@ -306,12 +308,12 @@ private static Dictionary<V, double> MatchWithSubset(Dictionary<V,double> search | |
foreach (var ele in subset) | ||
{ | ||
//if any element in tagDictionary matches to any element in subset, return true | ||
if (currentElementName.IndexOf(ele.FullName) != -1) | ||
if (currentElementName.IndexOf(ele.FullName, StringComparison.OrdinalIgnoreCase) != -1) | ||
{ | ||
filteredDict.Add(searchElement.Key, searchElement.Value); | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
return filteredDict; | ||
} | ||
|
@@ -342,7 +344,7 @@ internal void RebuildTagDictionary() | |
tagAndWeight => | ||
new | ||
{ | ||
Tag = tagAndWeight.Key, | ||
Tag = tagAndWeight.Key.Substring(0, tagAndWeight.Key.Length > LIMIT_SEARCH_TAG_SIZE ? LIMIT_SEARCH_TAG_SIZE : tagAndWeight.Key.Length), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this actually useful at this point? Does the cost of all these extra conditional operations undo the potential savings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same q here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well...after all search tags have been added/removed |
||
Weight = tagAndWeight.Value, | ||
Entry = entryAndTags.Key | ||
})) | ||
|
@@ -379,12 +381,13 @@ internal IEnumerable<V> Search(string query, int minResultsForTolerantSearch = 0 | |
query = query.ToLower(); | ||
|
||
var subPatterns = SplitOnWhiteSpace(query); | ||
|
||
// Add full (unsplit by whitespace) query to subpatterns | ||
var subPatternsList = subPatterns.ToList(); | ||
subPatternsList.Insert(0, query); | ||
subPatterns = (subPatternsList).ToArray(); | ||
|
||
if (subPatterns.Length > 1)// More than one word | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
{ | ||
// Add full (unsplit by whitespace) query to subpatterns | ||
var subPatternsList = subPatterns.ToList(); | ||
subPatternsList.Insert(0, query); | ||
subPatterns = (subPatternsList).ToArray(); | ||
Comment on lines
+405
to
+407
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain with an example? If the query is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is correct. |
||
} | ||
|
||
foreach (var pair in tagDictionary.Where(x => MatchWithQueryString(x.Key, subPatterns))) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,8 +105,7 @@ | |
Text="{Binding Path=SearchText, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" | ||
MinWidth="200" | ||
CaretBrush="{StaticResource CommonSidebarTextColor}" | ||
Margin="26,0,0,-1" | ||
TextChanged="OnSearchTextBoxTextChanged" /> | ||
Margin="26,0,0,-1"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason the twoWay Binding to "SearchText" + TextChanged="OnSearchTextBoxTextChanged" causes 2 TextChanged events to be triggered for every input character So I changed it so that we only bind the Text control to the "SearchText" Path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could this also be fixed by making this a one way binding? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing around the binding types broke other parts of the UI (ex. making it one way did not update the Textbox initial value "Search" anymore) |
||
</Grid> | ||
</Border> | ||
</Grid> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,10 +37,16 @@ public virtual void OnRequestFocusSearch() | |
} | ||
|
||
public event EventHandler SearchTextChanged; | ||
|
||
/// <summary> | ||
/// Invokes the SearchTextChanged event handler and executes the SearchCommand | ||
/// </summary> | ||
public void OnSearchTextChanged(object sender, EventArgs e) | ||
{ | ||
if (SearchTextChanged != null) | ||
SearchTextChanged(this, e); | ||
|
||
SearchCommand?.Execute(null); | ||
} | ||
|
||
#endregion | ||
|
@@ -65,13 +71,13 @@ public bool BrowserVisibility | |
set { browserVisibility = value; RaisePropertyChanged("BrowserVisibility"); } | ||
} | ||
|
||
private string searchText; | ||
/// <summary> | ||
/// SearchText property | ||
/// </summary> | ||
/// <value> | ||
/// This is the core UI for Dynamo, primarily used for logging. | ||
/// </value> | ||
private string searchText; | ||
public string SearchText | ||
{ | ||
get { return searchText; } | ||
|
@@ -208,8 +214,6 @@ internal void Filter() | |
} | ||
strBuilder.Append(", "); | ||
} | ||
|
||
Analytics.LogPiiInfo("Filter-categories", strBuilder.ToString().Trim()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is this triggered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Analytics.LogPiiInfo is deprecated and actually does not log anything anymore. I just cleaned up the SearchViewModel class ...I do not think performance is affected by this though.. |
||
} | ||
|
||
/// <summary> | ||
|
@@ -822,8 +826,6 @@ public void SearchAndUpdateResults(string query) | |
if (Visible != true) | ||
return; | ||
|
||
Analytics.LogPiiInfo("Search", query); | ||
|
||
// if the search query is empty, go back to the default treeview | ||
if (string.IsNullOrEmpty(query)) | ||
return; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Dynamo.Search; | ||
|
@@ -8,7 +10,7 @@ | |
namespace Dynamo.Tests.Search | ||
{ | ||
[TestFixture] | ||
class SearchDictionaryTest | ||
class SearchDictionaryTest : UnitTestBase | ||
{ | ||
/// <summary> | ||
/// This test method will execute several Add methods located in the SearchDictionary class | ||
|
@@ -57,6 +59,43 @@ public void TestAddItems() | |
Assert.AreEqual(searchDictionary.NumTags, 11); | ||
} | ||
|
||
[Test] | ||
[Category("UnitTests")] | ||
public void TestSearchDictionaryPerformance() | ||
{ | ||
//Arrange | ||
var searchDictionary = new SearchDictionary<string>(); | ||
|
||
var tagsPath = System.IO.Path.Combine(TestDirectory, "performance", "search_tags", "searchtags.log"); | ||
var tags = System.IO.File.ReadAllLines(tagsPath); | ||
int value = 0; | ||
foreach (var tag in tags) | ||
{ | ||
searchDictionary.Add($"Value:{value++}", tag); | ||
} | ||
|
||
Stopwatch stopwatch = new Stopwatch(); | ||
stopwatch.Start(); | ||
|
||
var query1 = "all"; | ||
searchDictionary.Search(query1); | ||
var query2 = "all elements"; | ||
searchDictionary.Search(query2); | ||
var query3 = "all elements of"; | ||
searchDictionary.Search(query3); | ||
var query4 = "all elements of category"; | ||
searchDictionary.Search(query4); | ||
var query = "az"; | ||
var results = searchDictionary.Search(query); | ||
|
||
stopwatch.Stop(); | ||
|
||
Assert.AreEqual(results.Count(), 20); | ||
|
||
int timeLimit = 260;//ms | ||
Assert.IsTrue(Math.Abs(stopwatch.ElapsedMilliseconds - timeLimit) < 0.2 * timeLimit, $"Search time should be within a range of +/- 20% of {timeLimit}ms but we got {stopwatch.ElapsedMilliseconds}ms"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still might be a bit flaky ... |
||
} | ||
|
||
/// <summary> | ||
/// This test method will execute several Remove methods located in the SearchDictionary class | ||
/// int Remove(Func<V, bool> removeCondition) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we all agree that 300 characters is good enough ?
Should we increase ?
Looking through the search tags we got from Jostein, I found the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALso this limit will be used in the Package Search window as well..(using package descriptions)
We could use another limit for Package Search ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @mjkkirschner @aparajit-pratap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine by me but I'm still confused - these search tags from Jostein look like node descriptions. I'm curious if the entire description is used as a search tag, then what's the benefit of the search tags included in the
<search></search>
XML?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aparajit-pratap it's to inject extra search terms without adding them to the description.
So I can add
box
to the search terms for Cuboid - without needing to addbox
to the description.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinzart90 what do you think about making this a hidden preference? I know it's more work, and kind of a pain, but it let's users tweak it if they run into trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjkkirschner I added a new assembly config for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, sorry @pinzart90 - I was not clear, by hidden preference -I just meant a preference in the dynamosettings.xml with no analog in the UI to control it. This is fine as well, users can still modify this if we want them to test something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjkkirschner if you think there is good enough reason to move to Preferences, then I can do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to move it to the preference settings file so we have all preferences in one place. That is usually the one file where all such user preferences go.