Node Search performance improvements by pinzart90 · Pull Request #12056 · DynamoDS/Dynamo

pinzart90 · 2021-09-23T16:46:18Z

Simplify search to improve performance
Changes:

Fix double search event caused by UI events
Do not add the whole search text if there is only one word (using whitespace as separator)
Limit search to 300 characters
use OrdinalIgnoreCase option in IndexOf calls

Note:
Difference between culture sensitive and ordinal serarch:
https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#string-comparisons-that-use-the-current-culture

I would opt for OrdinalIgnoreCase because we get better performance...and the order of the returned results is not really a deal breaker (I think...)

pinzart90 · 2021-09-23T18:31:44Z

src/DynamoCoreWpf/Controls/IncanvasSearchControl.xaml

                             CaretBrush="{StaticResource CommonSidebarTextColor}"
-                             Margin="26,0,0,-1"
-                             TextChanged="OnSearchTextBoxTextChanged" />
+                             Margin="26,0,0,-1"/>


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

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

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)

mjkkirschner · 2021-09-23T19:53:28Z

src/DynamoCore/Search/SearchDictionary.cs

    public class SearchDictionary<V>
    {
        private ILogger logger;
+        private static int LIMIT_SEARCH_TAG_SIZE = 100;


could this be too small for some localized searches?

Maybe...the value is definitely up for debate. I wonder if it would actually be best to warn the user if this limit is surpassed (I am not sure when or where)
I can take a look at the tags we got from Jostein to get a better idea of how many characters there are in a "normal" situation

Increased to 300

mjkkirschner · 2021-09-23T19:54:40Z

src/DynamoCore/Search/SearchDictionary.cs

                                new
                                {
-                                    Tag = tagAndWeight.Key,
+                                    Tag = tagAndWeight.Key.Substring(0, tagAndWeight.Key.Length > LIMIT_SEARCH_TAG_SIZE ? LIMIT_SEARCH_TAG_SIZE : tagAndWeight.Key.Length),


is this actually useful at this point? Does the cost of all these extra conditional operations undo the potential savings?

Same q here.

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.

mjkkirschner · 2021-09-23T19:55:29Z

src/DynamoCore/Search/SearchDictionary.cs

-            subPatternsList.Insert(0, query);
-            subPatterns = (subPatternsList).ToArray();
-
+            if (subPatterns.Length > 1)// More than one word


mjkkirschner · 2021-09-23T19:57:04Z

@pinzart90 it looks good after the tests pass, can I ask you to manually check that regular library search still works?

Amoursol · 2021-09-23T20:31:12Z

@pinzart do we have any metrics on how much better search is before and after this 🙏 ?

aparajit-pratap · 2021-09-23T21:43:48Z

src/DynamoCore/Search/SearchDictionary.cs

+                var subPatternsList = subPatterns.ToList();
+                subPatternsList.Insert(0, query);
+                subPatterns = (subPatternsList).ToArray();


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"]?

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

pinzart90 · 2021-09-24T14:41:40Z

@mjkkirschner I am working now on adding a performance test...and I will also check that existing functionality is not broken
@Amoursol I will get you those numbers

pinzart90 · 2021-09-27T19:24:31Z

Here are some numbers @Amoursol
These have been generated on my machine

With Fix "all elements of category" 5 times:
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 386ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 397ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 368ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 381ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 378ms -> 2

Without Fix "all elements of category" 5 times:
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 854ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 822ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 863ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 936ms -> 2
Searching for: "all elements of category", [Entries:1254, Tags:28110] : 869ms -> 2

With Fix "category" 5 times:
Searching for: "category", [Entries:1254, Tags:28110] : 78ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 80ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 71ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 66ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 71ms -> 20

Without Fix "category" 5 times:
Searching for: "category", [Entries:1254, Tags:28110] : 309ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 348ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 290ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 291ms -> 20
Searching for: "category", [Entries:1254, Tags:28110] : 282ms -> 20

pinzart90 · 2021-09-27T19:25:57Z

test/DynamoCoreTests/Search/SearchDictionaryTest.cs

+            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");


Still might be a bit flaky ...

pinzart90 · 2021-09-27T19:28:07Z

src/DynamoCore/Search/SearchDictionary.cs

    public class SearchDictionary<V>
    {
        private ILogger logger;
+        private static int LIMIT_SEARCH_TAG_SIZE = 300;


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:

The longest search tag is 899 characters

There are 34 search tags (out of 6525) that are over 300 characters

ALso this limit will be used in the Package Search window as well..(using package descriptions)
We could use another limit for Package Search ....

FYI @mjkkirschner @aparajit-pratap

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?

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

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

@mjkkirschner I added a new assembly config for it

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.

@mjkkirschner if you think there is good enough reason to move to Preferences, then I can do that

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.

Amoursol · 2021-09-27T19:39:01Z

Here are some numbers @Amoursol
These have been generated on my machine...

Ok awesome, so that looks to be roughly a:

56% performance boost on All Elements of Category
76% performance boost on Category

That's pretty epic! We can discuss further works after 2.13 drops :) Thanks Tibi.

aparajit-pratap · 2021-09-28T19:07:26Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

                strBuilder.Append(", ");
            }
-
-            Analytics.LogPiiInfo("Filter-categories", strBuilder.ToString().Trim());


When is this triggered?

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

aparajit-pratap · 2021-09-28T19:22:21Z

src/DynamoCore/Search/SearchDictionary.cs

    public class SearchDictionary<V>
    {
        private ILogger logger;
+        private static int LIMIT_SEARCH_TAG_SIZE = 300;


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?

aparajit-pratap · 2021-10-01T17:00:32Z

src/DynamoCore/Search/SearchDictionary.cs

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


Is this the DynamoCore.dll.config file?

pinzart90 added 4 commits September 23, 2021 12:45

first pass

06d0ad3

Update SearchDictionary.cs

ec2fd27

Update SearchDictionary.cs

4ad7d64

Update SearchDictionary.cs

a4a0298

pinzart90 changed the title ~~first pass~~ Node Search performance improvements Sep 23, 2021

pinzart90 requested review from aparajit-pratap, mjkkirschner and sm6srw September 23, 2021 18:28

pinzart90 added the PTAL Please Take A Look 👀 label Sep 23, 2021

pinzart90 commented Sep 23, 2021

View reviewed changes

update

ca08dd7

pinzart90 marked this pull request as ready for review September 23, 2021 18:50

mjkkirschner reviewed Sep 23, 2021

View reviewed changes

mjkkirschner approved these changes Sep 23, 2021

View reviewed changes

aparajit-pratap reviewed Sep 23, 2021

View reviewed changes

aparajit-pratap approved these changes Sep 24, 2021

View reviewed changes

pinzart90 added 7 commits September 27, 2021 09:55

improvements

6344b71

Merge branch 'master' into simplify_search

6f1341d

add unit performance unit test

3c4eca2

test seachCommand invoke count

cc1db45

Create searchtags.log

7124644

update

ebe06b4

scramble data

7be230c

pinzart90 commented Sep 27, 2021

View reviewed changes

pinzart90 requested review from aparajit-pratap and mjkkirschner September 27, 2021 19:32

Update SearchDictionaryTest.cs

ab05505

aparajit-pratap reviewed Sep 28, 2021

View reviewed changes

aparajit-pratap approved these changes Sep 28, 2021

View reviewed changes

mjkkirschner approved these changes Sep 28, 2021

View reviewed changes

pinzart90 added 2 commits October 1, 2021 08:08

Merge branch 'master' into simplify_search

ce3999d

add hidden option

3216447

aparajit-pratap reviewed Oct 1, 2021

View reviewed changes

aparajit-pratap approved these changes Oct 1, 2021

View reviewed changes

pinzart90 merged commit f9c7691 into master Oct 4, 2021

pinzart90 deleted the simplify_search branch October 4, 2021 13:06

Conversation

pinzart90 commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinzart90 Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Sep 23, 2021

Uh oh!

Amoursol commented Sep 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinzart90 commented Sep 24, 2021

Uh oh!

pinzart90 commented Sep 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinzart90 Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amoursol commented Sep 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

pinzart90 commented Sep 23, 2021 •

edited

Loading

pinzart90 Sep 23, 2021 •

edited

Loading

mjkkirschner Sep 23, 2021 •

edited

Loading

pinzart90 Sep 27, 2021 •

edited

Loading

mjkkirschner Sep 28, 2021 •

edited

Loading