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

Node Search performance improvements #12056

Merged
merged 15 commits into from Oct 4, 2021
41 changes: 31 additions & 10 deletions src/DynamoCore/Search/SearchDictionary.cs
Expand Up @@ -4,6 +4,8 @@
using System.Collections.Generic;
using System.Linq;
using System.Diagnostics;
using System.Configuration;
using System.Reflection;

namespace Dynamo.Search
{
Expand All @@ -13,6 +15,23 @@ namespace Dynamo.Search
public class SearchDictionary<V>
{
private ILogger logger;
private static int LIMIT_SEARCH_TAG_SIZE = 300;
Copy link
Contributor Author

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:

  1. The longest search tag is 899 characters
  2. There are 34 search tags (out of 6525) that are over 300 characters

Copy link
Contributor Author

@pinzart90 pinzart90 Sep 27, 2021

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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?

Copy link
Member

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 add box to the description.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.


static SearchDictionary()
{
try
{
// Look up search tag limit in the assembly configuration
var assemblyConfig = ConfigurationManager.OpenExeConfiguration(Assembly.GetExecutingAssembly().Location);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the DynamoCore.dll.config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

if (assemblyConfig != null)
{
var searchTagSizeLimit = assemblyConfig.AppSettings.Settings["searchTagSizeLimit"];
if (searchTagSizeLimit != null && int.TryParse(searchTagSizeLimit.Value, out int value))
LIMIT_SEARCH_TAG_SIZE = value;
}
}
catch { }
}

/// <summary>
/// Construct a SearchDictionary object
Expand Down Expand Up @@ -277,7 +296,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;
Expand Down Expand Up @@ -306,12 +326,12 @@ private static bool MatchWithQueryString(string key, string[] subPatterns)
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;
}
Expand Down Expand Up @@ -342,7 +362,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),
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same q here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well...after all search tags have been added/removed RebuildTagDictionary is called only once.
Performance penalty here should be negligible, compared to the constant searches on each text change.

Weight = tagAndWeight.Value,
Entry = entryAndTags.Key
}))
Expand Down Expand Up @@ -379,12 +399,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
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain with an example? If the query is all elements of category, the subpatterns list will be ["all", "elements", "of", "category"], then the subpatterns list will now be ["all elements of category", "all", "elements", "of", "category"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is correct.
I think the logic behind it was to look for the full text first (a match with the full text should have the highest priority).

}

foreach (var pair in tagDictionary.Where(x => MatchWithQueryString(x.Key, subPatterns)))
{
Expand Down
1 change: 1 addition & 0 deletions src/DynamoCoreWpf/Commands/SearchCommands.cs
Expand Up @@ -14,6 +14,7 @@ public DelegateCommand FocusSearchCommand
public DelegateCommand SearchCommand
{
get { return search ?? (search = new DelegateCommand(Search, CanSearch)); }
internal set { search = value; }// used by tests
}

private DelegateCommand showSearch;
Expand Down
3 changes: 1 addition & 2 deletions src/DynamoCoreWpf/Controls/IncanvasSearchControl.xaml
Expand Up @@ -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"/>
Copy link
Contributor Author

@pinzart90 pinzart90 Sep 23, 2021

Choose a reason for hiding this comment

The 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.
When the text is changed, the "SearchText" property's setter will be called...and that is when we will execute the Search command

Copy link
Member

Choose a reason for hiding this comment

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

could this also be fixed by making this a one way binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Expand Down
12 changes: 1 addition & 11 deletions src/DynamoCoreWpf/Controls/IncanvasSearchControl.xaml.cs
Expand Up @@ -60,16 +60,6 @@ private void OnRequestShowInCanvasSearch(ShowHideFlags flags)
}
}

private void OnSearchTextBoxTextChanged(object sender, TextChangedEventArgs e)
{
BindingExpression binding = ((TextBox)sender).GetBindingExpression(TextBox.TextProperty);
if (binding != null)
binding.UpdateSource();

if (ViewModel != null)
ViewModel.SearchCommand.Execute(null);
}

private void OnMouseLeftButtonUp(object sender, MouseButtonEventArgs e)
{
var listBoxItem = sender as ListBoxItem;
Expand All @@ -86,7 +76,7 @@ private void ExecuteSearchElement(ListBoxItem listBoxItem)
if (searchElement != null)
{
searchElement.Position = ViewModel.InCanvasSearchPosition;
searchElement.ClickedCommand.Execute(null);
searchElement.ClickedCommand?.Execute(null);
Analytics.TrackEvent(
Dynamo.Logging.Actions.Select,
Dynamo.Logging.Categories.InCanvasSearchOperations,
Expand Down
Expand Up @@ -360,8 +360,6 @@ protected virtual void OnClicked()
{
var nodeModel = Model.CreateNode();
Clicked(nodeModel, Position);

Analytics.LogPiiInfo("Search-NodeAdded", FullName);
}
}

Expand Down
12 changes: 7 additions & 5 deletions src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs
Expand Up @@ -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
Expand All @@ -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; }
Expand Down Expand Up @@ -208,8 +214,6 @@ internal void Filter()
}
strBuilder.Append(", ");
}

Analytics.LogPiiInfo("Filter-categories", strBuilder.ToString().Trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

When is this triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Expand Down Expand Up @@ -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;
Expand Down
42 changes: 41 additions & 1 deletion test/DynamoCoreTests/Search/SearchDictionaryTest.cs
@@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -57,6 +59,44 @@ 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);

double timeLimit = 300;//ms
double range = 30;
Assert.IsTrue(Math.Abs(stopwatch.ElapsedMilliseconds - timeLimit) < range / 100 * timeLimit, $"Search time should be within a range of +/- {range}% of {timeLimit}ms but we got {stopwatch.ElapsedMilliseconds}ms");
}

/// <summary>
/// This test method will execute several Remove methods located in the SearchDictionary class
/// int Remove(Func<V, bool> removeCondition)
Expand Down
26 changes: 26 additions & 0 deletions test/DynamoCoreWpfTests/CoreUITests.cs
Expand Up @@ -4,6 +4,7 @@
using System.Linq;
using System.Threading;
using System.Windows;
using System.Windows.Controls.Primitives;
using System.Windows.Input;
using CoreNodeModels.Input;
using Dynamo.Configuration;
Expand All @@ -19,6 +20,7 @@
using Dynamo.Scheduler;
using Dynamo.Selection;
using Dynamo.Services;
using Dynamo.UI.Controls;
using Dynamo.Utilities;
using Dynamo.ViewModels;
using Dynamo.Views;
Expand Down Expand Up @@ -863,6 +865,30 @@ public void WorkspaceContextMenu_TestIfInCanvasSearchHidesOnOpeningContextMenu()
Assert.IsFalse(currentWs.InCanvasSearchBar.IsOpen);
}

[Test]
[Category("UnitTests")]
public void InCanvasSearchTextChangeTriggersOneSearchCommand()
{
var currentWs = View.ChildOfType<WorkspaceView>();

// open context menu
RightClick(currentWs.zoomBorder);

// show in-canvas search
ViewModel.CurrentSpaceViewModel.ShowInCanvasSearchCommand.Execute(ShowHideFlags.Show);

var searchControl = currentWs.ChildrenOfType<Popup>().Select(x => (x as Popup)?.Child as InCanvasSearchControl).Where(c => c != null).FirstOrDefault();
Assert.IsNotNull(searchControl);

int count = 0;
(searchControl.DataContext as SearchViewModel).SearchCommand = new Dynamo.UI.Commands.DelegateCommand((object _) => { count++; });
searchControl.SearchTextBox.Text = "dsfdf";


Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen);
Assert.AreEqual(count, 1);
}

[Test]
[Category("UnitTests")]
public void WorkspaceContextMenu_TestIfSearchTextClearsOnOpeningContextMenu()
Expand Down