Search bar stuttering improvements by bendikberg · Pull Request #15726 · DynamoDS/Dynamo

bendikberg · 2024-12-16T10:14:43Z

Purpose

The Dynamo search bar feels stuttery when typing many characters in succession, as every key stroke starts a new search. These searches then block the Dynamo UI and will in some cases leave the UI hanging for multiple seconds at a time while all the searches complete. Since the user most likely didn't want to search for 'e', 'el', 'ele', ... etc. when they typed 'element', this both breaks user flow and leads to search feeling way slower than it has to be.

This change adds a debounce to the search (which could potentially be configurable by the user), which postpones the search until the user has paused typing for a short while.

Current search

xSuE3gCjMb.mp4

Proposed change

mfDyPu2TBW.mp4

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)
@mjkkirschner

(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
@dimven

…ry key stroke

github-actions · 2024-12-16T15:33:37Z

UI Smoke Tests

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

mjkkirschner · 2024-12-17T16:00:48Z

@bendikberg - do you have access to this link?
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/16920/testReport/junit/DynamoCoreWpfTests/CoreUserInterfaceTests/InCanvasSearchTextChangeTriggersOneSearchCommand/ or ADSK VPN?

src/DynamoCoreWpf/Utilities/ActionDebouncer.cs

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

mjkkirschner · 2024-12-17T16:14:45Z

src/DynamoCoreWpf/Utilities/ActionDebouncer.cs

+            if (_cts != null)
+                _cts.Cancel();
+            _cts = new CancellationTokenSource();
+            System.Threading.Tasks.Task.Delay(timeout, _cts.Token).ContinueWith(t =>


hmm, unfortunately I find this hard to reason about, it seems to me that the continuation will execute on a thread pool thread, not the main UI thread - which means that the dispatcher invoke call will be made from another thread. I think you should use BeginInvoke to avoid any super hard to reason about deadlocks.

Changed to use BeginInvoke. Maybe it could benefit from explicitly setting the DispatcherPriority as well?

Invoke and BeginInvoke should both execute on the dispatcher's associated thread (in our case the UI thread).
It should not matter if the dispatcher invoke calls are coming from other threads. Performance could be an issue (thread switching)...
We could also use the run the Continuation on the SynchronizationContext.Current (which in WPF apps, by default, is always associated with the UI thread)

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

mjkkirschner · 2024-12-17T16:19:51Z

Thanks for trying to address this!

besides the review comments - some more thoughts.

Because this is a UI change I think sticking it behind a feature flag would be a great idea.
Because if I'm right - it might introduce some complexity, even though it looks simple, I think a way to disable it / a feature flag is a good idea again.
The failing test needs to be updated and the new class needs tests.

@QilongTang do you agree on point 1 and 2? @bendikberg you can check feature flags here:

Dynamo/src/DynamoUtilities/DynamoFeatureFlagsManager.cs

Line 93 in 7bcdb04

internal T CheckFeatureFlag<T>(string featureFlagKey, T defaultval)

but we'd need someone to add a feature flag for debug and release builds online for you to check.

QilongTang · 2024-12-17T17:04:53Z

Thanks for trying to address this!

besides the review comments - some more thoughts.

Because this is a UI change I think sticking it behind a feature flag would be a great idea.

Because if I'm right - it might introduce some complexity, even though it looks simple, I think a way to disable it / a feature flag is a good idea again.

The failing test needs to be updated and the new class needs tests.

@QilongTang do you agree on point 1 and 2? @bendikberg you can check feature flags here:

Dynamo/src/DynamoUtilities/DynamoFeatureFlagsManager.cs

Line 93 in 7bcdb04

internal T CheckFeatureFlag<T>(string featureFlagKey, T defaultval)

but we'd need someone to add a feature flag for debug and release builds online for you to check.

Fine by me, do you want me to create a flag for him, or a flag for the overall performance improvements?

mjkkirschner · 2024-12-17T19:26:43Z

Thanks for trying to address this!
besides the review comments - some more thoughts.

Because this is a UI change I think sticking it behind a feature flag would be a great idea.

Because if I'm right - it might introduce some complexity, even though it looks simple, I think a way to disable it / a feature flag is a good idea again.

The failing test needs to be updated and the new class needs tests.

@QilongTang do you agree on point 1 and 2? @bendikberg you can check feature flags here:

Dynamo/src/DynamoUtilities/DynamoFeatureFlagsManager.cs

Line 93 in 7bcdb04

internal T CheckFeatureFlag<T>(string featureFlagKey, T defaultval)

but we'd need someone to add a feature flag for debug and release builds online for you to check.

Fine by me, do you want me to create a flag for him, or a flag for the overall performance improvements?

Let’s do granular flags for each change. Thanks Aaron.

…ude debouncer testing

bendikberg · 2024-12-18T14:10:22Z

Thanks for the input @QilongTang and @mjkkirschner

I tried implementing the suggested changes, as well as update the test to include some debounced cases as well. Should those be in separate functions instead?

I am a little unsure what to do about the feature flag, will you set one up for this specific case?
Is there a place that I can look up available feature flags?

bendikberg · 2024-12-18T14:13:04Z

test/DynamoCoreWpfTests/CoreUITests.cs

+            // run with debouncer
+            count = 0;
+            searchControl.SearchTextBox.Text = "dsfdfdsfdf";
+            Thread.Sleep(vm.searchDelayTimeout * 2);


Are Thread.Sleep()'s okay during tests? I'm not sure how else to verify that the debounces work as expected.

I think it's acceptable since these are relatively small. - I guess you could also make the delay time even smaller for testing though too small and it's probably not reliable anyway.

mjkkirschner · 2024-12-18T14:35:50Z

test/DynamoCoreWpfTests/CoreUITests.cs


            int count = 0;
-            (searchControl.DataContext as SearchViewModel).SearchCommand = new Dynamo.UI.Commands.DelegateCommand((object _) => { count++; });
+            var vm = searchControl.DataContext as SearchViewModel;


would you mind splitting this into 2 tests?

oh, I missed this ..

mjkkirschner · 2024-12-18T14:46:40Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

-        private string searchText;
+        internal int searchDelayTimeout = 150;
+        private bool useDebouncer = true; // replace with feature flag check
+        private ActionDebouncer searchDebouncer = new ActionDebouncer(Dispatcher.CurrentDispatcher);


so you have to be a bit careful using method of retrieving the dispatcher - if this code is called from a non ui thread the dispatcher will not be the WPF dispatcher.

That seems unlikely in most scenarios. If you have access to the DynamoViewModel here there is a property UIDispatcher that should always be correct, as its set from the WPF main window's dispatcher.

Yeah, that Dispatcher retrieval issue was the original reason for having the initialization inside the SearchText setter, with the assumption that the setter would only be called from the UI thread.

The DynamoViewModel UIDispatcher seems to be set by the View it belongs to after the SearchViewModel is constructed, so that requires the debouncer to be constructed lazily at some point in the future when the UIDispatcher has been set, rather than during the SearchViewModel construction.

So then I think we're back to some variation on the first version with a null check inside of the SearchText-setter?

Yeah, that Dispatcher retrieval issue was the original reason for having the initialization inside the SearchText setter, with the assumption that the setter would only be called from the UI thread. ah ok.

I'm not sure that's a strong assumption. It's a public method on a public class running in an application full of extensions and packages.

For now, what about

moving it back to your original initialization with a comment

wrapping this call in a try catch and logging, it seems unlikely, but possible it could be a non-UI thread dispatcher and, in that case, it might crash when the search results get modified from the wrong thread - unless this only causes binding updates anyhow that will get marshalled to the UI thread by WPF?

That's fair, it probably was the wrong assumption on my part.

Alright, I went back to the lazy initialization.

I wrapped the call as suggested. I don't think the debouncer can ever use anything other than UIDispatcher now, unless someone sets the wrong dispatcher on the DynamoViewModel, but then I think bigger issues are in store.

I did try setting up a test where the Dispatcher gets set from another thread, and the SearchText is set from that same thread, and then again set from inside. It all works, even with the wrong Dispatcher.

Where it does fail is if I try to set the Text property directly on the control, which throws an exception due to using the wrong Dispatcher, but I don't think that has anything to do with the debouncer dispatcher.

mjkkirschner · 2024-12-18T17:02:50Z

test failed with:
changing the text multiple times in quick succession should cause a single update once timeout expires
Expected: 1
But was: 0

mjkkirschner · 2024-12-18T17:06:53Z

I've created searchbar_debounce - it's a bool flag.

It will return true in debug builds and false in release builds.
You can add it to the mock values that are returned from the feature flags manager in test mode, or you can skip the flag check in test mode as our tests are run on release builds.

bendikberg · 2024-12-19T15:32:04Z

I tried increasing the test debounce timeout somewhat, to see if that helps when running in CI. Since it returns 0 none of the debounces were allowed to finish before the sleep expired.

When running locally the average time of running a test is about 5 seconds with the window opening and closing, and the sleeps are at most 500 ms for one test.

mjkkirschner · 2024-12-19T15:51:13Z

I tried increasing the test debounce timeout somewhat, to see if that helps when running in CI. Since it returns 0 none of the debounces were allowed to finish before the sleep expired.

When running locally the average time of running a test is about 5 seconds with the window opening and closing, and the sleeps are at most 500 ms for one test.

so does your test work locally? Does it work if you run a bunch of view tests in serial in one test job?

bendikberg · 2024-12-19T15:57:17Z

The 3 tests I've touched work locally, yes (also as part of a bigger group).

mjkkirschner · 2024-12-19T16:03:49Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

            DefineFullCategoryNames(LibraryRootCategories, "");
            InsertClassesIntoTree(LibraryRootCategories);
+
+            useDebouncer = DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_debounce", false) ?? false;


I am not sure how often this class is instantiated, or if it's only once during dynamo view model creation.
If it's frequent, then I think this use of CheckFeatureFlag is safe - if it's only once at startup then I think it will be more predictable to either:

check the feature flag onTextChanged, it's probably safe to cache at that point, or don't if it does not affect performance.

use the static DynamoFeatureFlagsManager.FlagsRetrieved event to make the check only after we have received flags.... unfortunately, If you try to use that from this class's constructor - I think you could get into a situation where you then subscribe this event too late, it would need to be done before the feature flags retrieval is made, which is from dynamo model. If you want to only make this check once you could lump it into here:

Dynamo/src/DynamoCore/Models/DynamoModel.cs

Line 1015 in 23bf463

private void HandleFeatureFlags()

It's not obvious, but CheckFeatureFlag will only check the local cache of flags which may not have been retrieved yet from the flags service.

Personally, I would go for 1 because it's simpler to reason about IMO, touches less files etc unless performance is an issue but under the hood it's just a dictionary lookup.

mjkkirschner · 2024-12-20T02:57:48Z

@pinzart can you help me out here the tests are giving us a bunch of random failures, webview2 initialization errors, random libG loading errors... and I'm suspiciously eyeing the dispatcher do events stuff - can you remind me and @bendikberg of best practices when using that?

src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs

pinzart90 · 2024-12-20T05:10:48Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

+                    // Dipatcher.CurrentDispatcher has a chance of returning the wrong dispatcher, so we need to
+                    // use the dynamoViewModel.UIDispatcher. The UIDispatcher is set after the SearchViewModel
+                    // constructor runs, otherwise the debouncer could be initalized there
+                    searchDebouncer = new ActionDebouncer(dispatcher);


why not initialize this class in the SearchViewModel constructor ?
You could also pass the logger to the ActionDebouncer instance and do the try catch inside there (I am assuming this class is supposed to be reused through out dynamo)

pinzart90 · 2024-12-20T05:55:36Z

src/DynamoCoreWpf/Utilities/ActionDebouncer.cs

+                    dispatcher.BeginInvoke(action);
+                }
+            });
+        }


If you simply assume (document) that this class should only be called on the UI thread (not enforced) if it updates UI elements, you could simply schedule the continuation on the same thread by using the TaskScheduler.FromCurrentSynchronizationContext() option.
Note: TaskContinuationOptions.ExecuteSynchronously does not guarantee runing on the same thread.
This way you do not even need the dispatcher.
Passing a generic dispatcher to the constructor does not really enforce it being use in the UI thread anyway.

that sounds like a really good simplification.

I decided to go with async await instead of continuations. After reading some more about it, it seems async/await has better performance and allocates less resources

Changed my mind on the async/await vs continuations. Async/Await throws errors and is a nuisance during debugging. The difference in performance is at the level of microseconds and should not be an issue

pinzart90 · 2024-12-20T05:59:37Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

+                // To prevent any debounces from triggering at some unexpected point before or after the control
+                // becomes visible, this flag is only set once the searchText value is set by the user
+                // (unless it somehow gets set somewhere else)
+                searchInitialized = searchInitialized || !string.IsNullOrEmpty(searchText);


why not just run the debouncer if (!string.IsNullOrEmpty(searchText)) and the non debounced version if the searchtext is empty or null ?
searchText is set multiple times before the control becomes visible and interactabl
Is it called with non empty content ?

pinzart90 · 2024-12-20T06:04:05Z

src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs

+        internal ActionDebouncer searchDebouncer;
+        private bool searchInitialized = false;
+
+        private string searchText = string.Empty;


I am hoping we can just keep searchDelayTimeout and searchDebouncer

pinzart90 · 2024-12-20T06:41:52Z

@pinzart can you help me out here the tests are giving us a bunch of random failures, webview2 initialization errors, random libG loading errors... and I'm suspiciously eyeing the dispatcher do events stuff - can you remind me and @bendikberg of best practices when using that?

I only see some python tests failing with a Load exception. There are also some structured exceptions thrown (which historically have been hard to figure out). I would suggest to run the tests and log onto the build machine. Maybe some test gets stuck (with a message box on screen) and messes up the rest.

Webview2 exceptions would lead more to misuses of DoEvents (probably would have to use DoEventsLoop until the desired UI state is achieved). @mjkkirschner can you point out the webview2 exceptions?

mjkkirschner · 2024-12-20T16:27:07Z

@pinzart can you help me out here the tests are giving us a bunch of random failures, webview2 initialization errors, random libG loading errors... and I'm suspiciously eyeing the dispatcher do events stuff - can you remind me and @bendikberg of best practices when using that?

I only see some python tests failing with a Load exception. There are also some structured exceptions thrown (which historically have been hard to figure out). I would suggest to run the tests and log onto the build machine. Maybe some test gets stuck (with a message box on screen) and messes up the rest.

Webview2 exceptions would lead more to misuses of DoEvents (probably would have to use DoEventsLoop until the desired UI state is achieved). @mjkkirschner can you point out the webview2 exceptions?

The webview 2 initialization issues were here:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/16929/

mjkkirschner · 2024-12-20T16:27:55Z

@bendikberg can you try using DoEventsLoop in your test instead of do events directly?

mjkkirschner · 2025-01-17T16:55:07Z

src/DynamoCoreWpf/Utilities/ActionDebouncer.cs

+namespace Dynamo.Wpf.Utilities
+{
+    /// <summary>
+    /// The ActionDebouncer class offers a means to redunce the number of UI notifications for a specified time.


redunce spelling

pinzart90 · 2025-01-20T10:09:16Z

Passed here https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/17049
(1 failure, the asm binary load SEH exception)

This reverts commit 7334501.

This reverts commit 47629b3.

This reverts commit db5512d.

Add debouncer to search bar to prevent it from blocking the UI on eve…

8c2a38f

…ry key stroke

QilongTang added this to the 3.5 milestone Dec 16, 2024

QilongTang added the performance label Dec 16, 2024

mjkkirschner self-assigned this Dec 17, 2024

mjkkirschner reviewed Dec 17, 2024

View reviewed changes

src/DynamoCoreWpf/Utilities/ActionDebouncer.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 17, 2024

View reviewed changes

src/DynamoCoreWpf/Utilities/ActionDebouncer.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Dec 17, 2024

View reviewed changes

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

mjkkirschner reviewed Dec 17, 2024

View reviewed changes

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

bendikberg added 2 commits December 18, 2024 14:57

Debouncer and search text property robustness and fixes

854ad0e

Update InCanvasSearchTextChangeTriggersOneSearchCommand tests to incl…

bedbc6a

…ude debouncer testing

bendikberg commented Dec 18, 2024

View reviewed changes

mjkkirschner reviewed Dec 18, 2024

View reviewed changes

Split InCanvasSearchTextChangeTriggersOneSearchCommand tests

99f92a8

bendikberg added 5 commits December 19, 2024 11:35

Add searchbar_debounce feature flag for tests

078de94

Disposable debouncer

8c72c7b

Lazy debouncer initialization with UIDispatcher, feature flag check

6c536cc

Increase test sleep time. Set useDebounce explicitly

89b50aa

Debounce from another thread test

ef6b609

mjkkirschner reviewed Dec 19, 2024

View reviewed changes

mjkkirschner mentioned this pull request Dec 19, 2024

DYN-8045: Fix hanged node search when the user types faster than search #15733

Closed

9 tasks

pinzart90 reviewed Dec 20, 2024

View reviewed changes

src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs Show resolved Hide resolved

pinzart90 reviewed Dec 20, 2024

View reviewed changes

pinzart90 added 3 commits December 20, 2024 18:49

Merge branch 'master' into pr/15726

0dd9242

Merge branch 'master' into pr/15726

2c6cb3d

update

2de5194

pinzart90 requested a review from mjkkirschner January 16, 2025 15:14

pinzart90 added 4 commits January 16, 2025 08:21

Update CoreUITests.cs

f92cf5d

Merge branch 'master' into pr/15726

0345dc7

update

2ed42c6

Update ActionDebouncer.cs

2e3f537

mjkkirschner approved these changes Jan 17, 2025

View reviewed changes

Update ActionDebouncer.cs

366816c

pinzart90 merged commit 7334501 into DynamoDS:master Jan 21, 2025
11 of 13 checks passed

QilongTang added a commit that referenced this pull request Jan 23, 2025

Revert "Search bar stuttering improvements (#15726)"

b2e32c9

This reverts commit 7334501.

pinzart90 added a commit that referenced this pull request Jan 23, 2025

Revert "Search bar stuttering improvements (#15726)"

47629b3

This reverts commit 7334501.

QilongTang pushed a commit that referenced this pull request Jan 23, 2025

Revert "Search bar stuttering improvements (#15726)" (#15778)

db5512d

pinzart90 added a commit that referenced this pull request Jan 27, 2025

Reapply "Search bar stuttering improvements (#15726)"

d475773

This reverts commit 47629b3.

pinzart90 added a commit that referenced this pull request Jan 28, 2025

Reapply "Search bar stuttering improvements (#15726)" (#15778)

e260bdc

This reverts commit db5512d.

Conversation

bendikberg commented Dec 16, 2024

Purpose

Current search

Proposed change

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

github-actions bot commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UI Smoke Tests

Uh oh!

mjkkirschner commented Dec 17, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjkkirschner Dec 17, 2024 • 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!

Uh oh!

mjkkirschner commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QilongTang commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Dec 17, 2024

Uh oh!

bendikberg commented Dec 18, 2024

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 Dec 18, 2024 • 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!

mjkkirschner commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendikberg commented Dec 19, 2024

Uh oh!

mjkkirschner commented Dec 19, 2024

Uh oh!

bendikberg commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner Dec 19, 2024 • 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!

mjkkirschner commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinzart90 Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

github-actions bot commented Dec 16, 2024 •

edited

Loading

mjkkirschner Dec 17, 2024 •

edited

Loading

mjkkirschner commented Dec 17, 2024 •

edited

Loading

QilongTang commented Dec 17, 2024 •

edited

Loading

mjkkirschner Dec 18, 2024 •

edited

Loading

mjkkirschner commented Dec 18, 2024 •

edited

Loading

mjkkirschner commented Dec 18, 2024 •

edited

Loading

bendikberg commented Dec 19, 2024 •

edited

Loading

mjkkirschner Dec 19, 2024 •

edited

Loading

mjkkirschner commented Dec 20, 2024 •

edited

Loading

pinzart90 Dec 20, 2024 •

edited

Loading

mjkkirschner commented Dec 20, 2024 •

edited

Loading