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

fix: Bad performance in Search Interactive when searching through thousands of items #389

Closed
Ogglas opened this issue May 11, 2023 · 13 comments

Comments

@Ogglas
Copy link
Contributor

Ogglas commented May 11, 2023

🐛 Bug Report

Search interactive is slow when searching through thousands of items.

https://www.fluentui-blazor.net/Search#interactive

Looking at Google Chrome Performance tab it seems Scripting related:

image

I think it has something to do with the rendering of FluentListbox. If this is removed handleSearchInput is handled very fast.

image

💻 Repro or Code Sample

@page "/SearchInteractive"

<FluentSearch @ref=searchTest 
              @oninput=handleSearchInput 
              @onchange=handleSearchInput
              @bind-Value="@searchValue"
              Placeholder="Search for name" />
<br />
<FluentListbox Items=@searchResults TOption="string" SelectedOptionChanged="@(e => searchValue = (e != defaultResultsText ? e : string.Empty) )"  />

<p>
    You searched for: @searchValue
</p>

<MatAutocompleteList Items="@searchData" TItem="string" Label="Pick one"></MatAutocompleteList>
@*https://www.matblazor.com/AutocompleteList*@

@code {
#nullable enable
    FluentSearch? searchTest;
    string? searchValue = string.Empty;
#nullable disable

    List<string> searchData = new()
    {
      //Add list here from https://sharetext.me/vfknowohwl. Due to Github "body is too long (maximum is 65536 characters)."
    };

    List<string> searchResults = defaultResults();

    static string defaultResultsText = "no results";
    static List<string> defaultResults()
    {
        return new() { defaultResultsText };
    }

    void handleSearchInput(ChangeEventArgs args)
    {
        if (args is not null && !string.IsNullOrWhiteSpace(args.Value.ToString()))
        {
            string searchTerm = args.Value.ToString()!.ToLower();

            if (searchTerm.Length > 0)
            {
                List<string> temp = searchData.Where(str => str.ToLower().Contains(searchTerm)).Select(str => str).ToList();
                if (temp.Count() > 0)
                {
                    searchResults = temp;
                }
            }
        }
        else
        {
            searchResults = defaultResults();
            searchValue = string.Empty;
        }
    }
}

🤔 Expected Behavior

Search interactive should work with thousands of items

💁 Possible Solution

A possible solution would be to add a timer to debounce the input like this:

https://blog.jeremylikness.com/blog/an-easier-blazor-debounce/#the-perfect-time

However given that we have all the data loaded it should not be needed. Tried with MatAutocompleteList and it has no lag with the same values.

🌍 Your Environment

OS & Device:: Windows 11 Pro on PC
Browser: Chrome Version 113.0.5672.64 (Official Build) (64-bit)
.NET 7 and Microsoft.Fast.Components.FluentUI V2.2.1.

@vnbaaij
Copy link
Collaborator

vnbaaij commented May 13, 2023

While I accept that the FluentListbox may have a performance issue (which I'll look at), in general I would say that this is not an advised way to work with it this way. If you have thousands of options, use an API to return your results.
Debouncing is a good idea to add to the example (that is what SearchInteractive is, just an example). Fancy supplying a PR for this?

@Ogglas
Copy link
Contributor Author

Ogglas commented May 15, 2023

Sounds good that you will look at any FluentListbox performance issues. I do however disagree with ways of working. If you have thousands of options and call an API it is even more important to have a debounce method. Otherwise you will have the current performance issues and on top of that a request to an API for every input you make. It would in this case make it even slower to work with.

There is an old issue for debounce but it was never implemented it seems. Given that they already have a debounce method it does not seem to be that hard to implement.

I will look at modifying the SearchInteractive example. If debounce is added natively it would be great if the component was updated as well.

Other unrelated findings:

For some reason you can only find the search component via URL and not in the components list. Is there any reason for this?
https://learn.microsoft.com/en-us/fluent-ui/web-components/components/search?pivots=typescript

Every Blazor example also redirects to https://brave-cliff-0c0c93310.azurestaticapps.net/ instead of https://www.fluentui-blazor.net/ for https://learn.microsoft.com/en-us/fluent-ui/web-components/.

@vnbaaij
Copy link
Collaborator

vnbaaij commented May 15, 2023

I wasn't questioning adding a debounce :), just having thousands of items in a listbox.

There is an old issue for debounce but it was never implemented it seems. Given that they already have a debounce method it does not seem to be that hard to implement.

That one was/is for the React Fluent UI components so that is unfortunately no good for us. It still seems to be on the roadmap for .NET 8 (dotnet/aspnetcore#10522) I'll see where that goes before adding something myself.

I just submitted a PR to the docs to add the Search component to the list and update all the sample links. Will take some days to flow through the system.

@Ogglas
Copy link
Contributor Author

Ogglas commented May 15, 2023

@vnbaaij I can absolutely agree on listbox items :)

Sounds good with your PR.

I have been looking at this today but something that bothers me is that FluentSearch currently uses event=onchange. Using a timer with timer.Elapsed I would prefer event=oninput.

Moving forward I would either like to modify the FluentSearch component or create a new component called FluentSearchInteractive that the SearchInteractive example would use:

@namespace Microsoft.Fast.Components.FluentUI
@inherits FluentSearch
<fluent-search @bind="CurrentValueAsString"
               @bind:event="oninput">
    @ChildContent
</fluent-search>

My component is based on this example:

https://learn.microsoft.com/en-us/aspnet/core/blazor/forms-and-input-components?view=aspnetcore-7.0#inputtext-based-on-the-input-event

I have tested the component locally and it works the way I expect it. Complete component created that works like the normal FluentSearch component but with bind:event="oninput":

@namespace Microsoft.Fast.Components.FluentUI
@inherits FluentSearch
<fluent-search @bind="CurrentValueAsString"
               @bind:event="oninput"
               @ref=Element
               class="@ClassValue"
               style="@StyleValue"
               readonly="@Readonly"
               autofocus="@Autofocus"
               placeholder="@Placeholder"
               list=@DataList
               maxlength="@Maxlength"
               minlength="@Minlength"
               pattern=@Pattern
               size="@Size"
               spellcheck="@Spellcheck"
               id=@Id
               current-value="@BindConverter.FormatValue(CurrentValue)"
               disabled="@Disabled"
               name=@Name
               required="@Required"
               appearance="@Appearance.ToAttributeValue()"
               @onchange="@(EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString))"
               @attributes="AdditionalAttributes">
    @ChildContent
</fluent-search>

What also works is simply adding the code below to FluentSearch component :

@oninput="@(EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString))"

If this is OK this is the best approach imo since there won't be a need to update several components. Is any of these approaches OK and which one do you prefer in that case?

@vnbaaij
Copy link
Collaborator

vnbaaij commented May 16, 2023

I prefer to have the 'overriden' approach where the oniput version is part of the demo site. I've done this for the number field as well

@Ogglas
Copy link
Contributor Author

Ogglas commented May 16, 2023

I would prefer to not take that approach. It means that I will have to import the component manually and update it myself if I wish to use it. Could we not add FluentNumberFieldOnInput and FluentSearchOnInput as regular components then? I see common usage scenarios for both.

Perhaps a better alternative is using an approach similar to MatBlazor and expose OnInput as a parameter for the original component? This would allow you to solve your problem while not having to maintain two components. Any current implementation will continue to work like it always has then.

https://www.matblazor.com/TextField

https://github.com/SamProf/MatBlazor/blob/master/src/MatBlazor/Components/MatTextField/MatInputTextComponent.razor

If any of the proposed solutions above for some reason are not OK then I can add it to examples/FluentUI.Demo.Shared/Shared/ next to FluentNumberFieldOnInput.

@vnbaaij
Copy link
Collaborator

vnbaaij commented May 16, 2023

I agree. We need to get them in to the library. I'll take a look at the MudBlazor approach and otherwise just AAD them as they are. Think the level of double work will be minimal.

What also works is simply adding the code below to FluentSearch component :

@oninput="@(EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString))"

Doing that will lead to double events being raised. Rather have separate components

@Ogglas
Copy link
Contributor Author

Ogglas commented May 17, 2023

After spending three days on this back and forth I think I have found a good solution. Given that you normally want to act on onchange as well when acting on oninput I decided to not write a new component at all in the end. This means that you only need one component, current functionality will stay the same, default is only one event raised but you can choose to opt in for oninput when needed.

See PR here: #396

Given that we talked about FluentNumberFieldOnInput I found a bug that in turn led to this PR.

I hope you will agree that both of these PRs are a good solution @vnbaaij.

@vnbaaij
Copy link
Collaborator

vnbaaij commented May 17, 2023

Very happy with this. PR is merged, Thanks Oscar! 💯 🥇

@vnbaaij vnbaaij closed this as completed May 17, 2023
@Ogglas
Copy link
Contributor Author

Ogglas commented May 17, 2023

Thank you @vnbaaij! Added some final fixes due to nullable reference types warnings

PR: #397

@supriya1586
Copy link

I tried many fixes from here. My problem is search functionality works fine. But search textbox doesnt work properly. when typed faster it eats up letter or behaves in any strage way possible. Can anyone help please?

@vnbaaij
Copy link
Collaborator

vnbaaij commented Jul 24, 2024

I tried many fixes from here. My problem is search functionality works fine. But search textbox doesnt work properly. when typed faster it eats up letter or behaves in any strage way possible. Can anyone help please?

This is more somethig for the discussion area, I think. Please create a new topic there.

@supriya1586
Copy link

sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants