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: Page with FluentButtons becomes slower and slower #871

Closed
dan67 opened this issue Oct 19, 2023 · 13 comments
Closed

fix: Page with FluentButtons becomes slower and slower #871

dan67 opened this issue Oct 19, 2023 · 13 comments
Labels
area:fluent-ui-web-components A FluentUI specific issue bug A bug status:blocked Any issue blocked by another

Comments

@dan67
Copy link

dan67 commented Oct 19, 2023

🐛 Bug Report

Page with a lot of buttons becomes slower and slower

💻 Repro or Code Sample

  1. dotnet new fluentuiblazorserver -n ButtonTest

  2. Update NuGet to 3.2.0

  3. Add ButtonPage:

@page "/buttons"
@using System.Diagnostics

<PageTitle>Buttons</PageTitle>

<h1>Buttons</h1>

<FluentCard class="button-page-content">
    @for (int i = 0; i < 2500; i++)
    {
        <div >
            <FluentButton Appearance="Appearance.Accent">Button</FluentButton>

            @* <FluentBadge Appearance="Appearance.Accent">Badge</FluentBadge> *@
        </div>
    }
</FluentCard>

@code {
    private Stopwatch _stopwatch;

    protected override void OnInitialized()
    {
        _stopwatch = Stopwatch.StartNew();
        base.OnInitialized();
    }

    protected override void OnAfterRender(bool firstRender)
    {
        base.OnAfterRender(firstRender);

        _stopwatch.Stop();
        Debug.WriteLine($"Rendering took {_stopwatch.ElapsedMilliseconds} ms");
    }
}
  1. Add ButtonPage to NavBar:
<li>
	<FluentAnchor Href="buttons" Appearance=@SetAppearance("ButtonsPage")>Buttons</FluentAnchor>
</li>
  1. Run project. Switch between "Counter" and "Buttons" in NavBar and notice in Debug Output that rendering takes longer and longer time:
    Rendering took 384 ms
    Rendering took 434 ms
    Rendering took 627 ms
    Rendering took 662 ms
    Rendering took 831 ms
    Rendering took 942 ms
    Rendering took 1124 ms
    Rendering took 1296 ms
    Rendering took 1439 ms
    Rendering took 1582 ms

🤔 Expected Behavior

Rendering time should stay the same.

😯 Current Behavior

Rendering time increases every time page is shown.

💁 Possible Solution

🔦 Context

Measuring performance

🌍 Your Environment

Windows 10 Enterprise
Visual Studio 17.7.4
Chrome 117
.NET SDK 7.0.401

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 22, 2023

Hi,

I ran your test code locally in 2 runs and got the following results:
1st run:
Rendering took 777 ms
Rendering took 961 ms
Rendering took 864 ms
Rendering took 3169 ms
Rendering took 946 ms
Rendering took 877 ms
Rendering took 765 ms
Rendering took 673 ms
Rendering took 662 ms

2nd run:
Rendering took 770 ms
Rendering took 1504 ms
Rendering took 813 ms
Rendering took 2982 ms
Rendering took 3202 ms
Rendering took 652 ms
Rendering took 713 ms
Rendering took 659 ms
Rendering took 684 ms
Rendering took 675 ms
Rendering took 743 ms

With the first run, I made a change in the code to not call the base. code as these calls are not necessary. It seem to make the outcome a bit more constant.

With the 2nd run, I did indeed see the times go up, but I was refreshing the page quite quickly. When I waited a bit after the longest rendering, the next runs were fast again. It seem like the .NET Garbage Collector just needs time to catch up (which is not surprising as we are creating 2500 components each run).
All and all, we think this is not an issue per se, but we are keeping an eye on this and plan to implement more benchmarking into the code in the future.

@dan67
Copy link
Author

dan67 commented Oct 23, 2023

Hi,
Removed calls to base and added GC to CTOR:

    private Stopwatch _stopwatch;

    public ButtonPage()
    {
        GC.Collect();
        Thread.Sleep(10000);
    }

    protected override void OnInitialized()
    {
        _stopwatch = Stopwatch.StartNew();
        // base.OnInitialized();
    }

    protected override void OnAfterRender(bool firstRender)
    {
        // base.OnAfterRender(firstRender);

        _stopwatch.Stop();
        Debug.WriteLine($"Rendering took {_stopwatch.ElapsedMilliseconds} ms");
    }

still the same result:
Rendering took 417 ms
Rendering took 482 ms
Rendering took 604 ms
Rendering took 707 ms
Rendering took 856 ms
Rendering took 1019 ms
Rendering took 1193 ms
Rendering took 1352 ms
Rendering took 1541 ms
Rendering took 1752 ms
Rendering took 1799 ms
Rendering took 1990 ms
Rendering took 2373 ms
Rendering took 2577 ms

When using FluentBadge instead of FluentButton the rendering time is stable.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 23, 2023

Are you testing it with a Server or a WebAssembly project?

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 23, 2023

I'm testing it with a Server project. Changed the code to this:

@code {
    private Stopwatch _stopwatch = new();

    protected override void OnInitialized()
    {
        _stopwatch = Stopwatch.StartNew();
    }

    protected override void OnAfterRender(bool firstRender)
    {
        _stopwatch.Stop();
        Console.WriteLine($"Rendering took {_stopwatch.ElapsedMilliseconds} ms");
    }

    public void Dispose()
    {
        Console.WriteLine("Dispose - Start with garbage collecting");
        GC.Collect();
        Console.WriteLine("Dispose - Done with garbage collecting");
    }
}

When just hitting refresh in the browser, I see the render time being stable. Also, the Dispose method is called twice then, but that is due to pre-rendering.
When I visit the page and then navigate to another page and repeat, I'm also seeing the rendering time increasing. Will investigate further.

@dan67
Copy link
Author

dan67 commented Oct 23, 2023

I was creating the project with dotnet new fluentuiblazorserver -n ButtonTest
(see step 1 in repro)

@andreisaperski
Copy link
Contributor

Looks like a memory leak in FAST lib - Detached Elements in dev tools shows that 2500 buttons become detached after each navigation from Buttons page. This happens because of:

  • FormAssociated.disconnectedCallback() doesn't call super.disconnectedCallback() (FASTElement).
  • MatchMediaStyleSheetBehavior (listens for 'forced-colors') is not unbinded in onDisconnectedCallback() of Controller - its listener is not removed from MediaQueryList. This happens because of PropertyStyleSheetBehavior which is unbined first and it also removes MatchMediaStyleSheetBehavior instance from 'behaviors'. Seems like a copy of 'this.behaviors' should be used for unbinding.
    With the issues above fixed, there are no Fluent buttons among detached elements after GC and rendering time doesn't increase.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Nov 2, 2023

Great find @andreisaperski! I'll make the FAST team aware.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Nov 9, 2023

@andreisaperski Do you perhaps have a minimal repo where the FAST team can look at to see the MatchMediaStyleSheetBehavior issue? Please see the linked issue in the fluentui repo above . Thanks.

@andreisaperski
Copy link
Contributor

andreisaperski commented Nov 9, 2023

@vnbaaij added a link to the repo and a bit more details there. the current FormAssociated fix is only a half of the memory leak fix.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 5, 2024

Closing this as the issue has been fixed on the FAST / Fluent UI side. New script version will be in next release (v4.5)

@vnbaaij vnbaaij closed this as completed Mar 5, 2024
@andreisaperski
Copy link
Contributor

@vnbaaij looks like it has been partially fixed and the memory leak is still there. MatchMediaStyleSheetBehavior is not unbined - after several nagivations to Button page (and back to Couter page) from the example and GC, there're 17.5k detached buttons:
image

Media query linsters ('forced-colors') are roots that prevent GC of buttons:
image

These listeners are created by MatchMediaStyleSheetBehavior:
image

@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 7, 2024

Yes, it is indeed only partially fixed...reopening it

@vnbaaij vnbaaij reopened this Mar 7, 2024
@vnbaaij
Copy link
Collaborator

vnbaaij commented May 29, 2024

Closing this as it will most probably not be fixed on the current FAST version anymore (with FAST element being archived)

@vnbaaij vnbaaij closed this as completed May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fluent-ui-web-components A FluentUI specific issue bug A bug status:blocked Any issue blocked by another
Projects
None yet
Development

No branches or pull requests

3 participants