chanan / BlazorStyled

CSS in Blazor Components
https://blazorstyled.io
The Unlicense
191 stars 19 forks source link

v3 performance #104

Open esso23 opened 4 years ago

esso23 commented 4 years ago

Just did some tests on existing code with v3.0.0 and it turns out that v3.0.0 is several times slower when loading than v2.2.0. I'm trying this on long lists of components with lots of nested components (in standard cases when there are not many classes it's working ok). I'm only using @bind-classname approach.

The components also load a lot of stuff from services and it seems like in v2.2.0 styles load first and then the stuff from services is fetched. In v v3.0.0 the stuff from services is already rendering while the styles are still not completely loaded.

chanan commented 4 years ago

That is really unexpected. Are these tests in a format you can share with me?

esso23 commented 4 years ago

Sadly I can't share the code since it's a commercial product and reproducing it would be quite time-consuming. Anyway, I played with profiling a bit and I traced the problem to CssAsync method (# 1 CPU killer) and then to this line:

https://github.com/chanan/BlazorStyled/blob/30b23f61e8510f9b3af2e47bf9525c271f6d4939/src/BlazorStyled/Internal/StyledImpl.cs#L42

In sync version you call Task.Run which means the code is not blocked by the call but in async version you await the call and the method is blocked until the JS interop is executed. I removed the await and I saw a performance boost immediately (since I render hundreds, if not thousands of classes at a time) - not saying that's a good solution - just proving the point.

I think for such scenarios it would be ideal to send CSS updates to clients in batches, or have some producer-consumer in a separate thread calling the JS stuff (although I don't know if that's possible in Blazor).

chanan commented 4 years ago

Would it be correct to assume that you are using the <Styled>...</Styled> tag?

esso23 commented 4 years ago

Yea, my only use case is this:

<BlazorStyled.Styled @bind-Classname="@SomeClass">
    css
</BlazorStyled.Styled>

<Component class="@SomeClass"></Component>

@code
{
    private string SomeClass;
}

I'm not using any other features of BlazorStyled anywhere in the code (except for keyframes, but that basically falls under this pattern aswell).

chanan commented 4 years ago

So I was going back and forth about whether <Styled /> should use the await the call to the javascript or not. Initially, (in some of the V3 alphas) I had it the other way around where it would just "fly" through the commands and return right away. I ended up going with the await because it help reduce "flicker" on load because you can hide your screen till all commands are done. But you are correct, it does slow down the processing.

Would adding a flag to <Styled /> be a good solution for you? Something like <Styled DoNotAwait="true">...</Styled> ? (I should point out that some cases might still need to await, but I am not sure I will need to test it out, but the "normal case it won't need to await"). You will lose the ability to know when all the CSS has loaded though.

esso23 commented 4 years ago

You can use ContinueWith() where you can trigger an event to show the component when the task finishes or "subscribe" to task completion in some other way. IMHO, having longer load times just to remove the "flicker" is a hack, or workaround at best, but not a proper solution.

chanan commented 4 years ago

Either way, I would prefer not to change the <Styled /> back to not awaiting. So will adding an attribute like mentioned above work for you?

esso23 commented 4 years ago

Hm, what I'm trying to say is that the current solution is not good and adding an attribute is not going to make it better. Making the methods async is good, but the fact that it makes the library slower is a big problem, since loading speed is an extremely important thing on web (if not the most important). So again, I think the solution is to not await that particular method at that particular place. The only reason it's done is to not display components before the classes are added at the cost of extreme slowdown of the overall process in some cases, which, again, is basically a hack.

esso23 commented 4 years ago

Just to paint the picture of what's the difference in my case. I measured the time since I hit the refresh button in the browser until everyhing gets rendered correctly: Without the await on that 1 line: 2.5 - 3.0 seconds With the await: 5.5 - 6.0 seconds That's on the page with highest amount of components in my current app (which isn't that crazy). The difference will get worse as the amount of components gets higher.

esso23 commented 4 years ago

Also, is it documented somewhere how to hide the component until all the styles are loaded?

chanan commented 4 years ago

I will answer the documentation piece first, its not documentated yet because I there is an issue with the docs site (and V3.0.0 in general that I posted here: https://github.com/chanan/BlazorStyled/issues/102)

In V3, the call is async because it calls out to Javscript. In V2, there was no call to Javascript, but had its own issues - such as needing to change the _host.cshtml which caused problems somestimes with other libraries people used, can't use stlesheet.insertRule so the full CSS text had to be outputed by blazor directly between <style /> tags, can't (easily) use more than one stylesheet, to name a few.

You mentioned that the version of IStyled.Css is faster than IStyled.CssAsync because of the Task.run()- so I would like to see if changing <Styled /> to use that version when you set an attribute. If that fixes the speed problem, that is the best solution. If it doesnt help, I am not sure how to proceed. One option is to use a hack that speed up sending data to javascript that was found by @lupusa1 mentioned on Twitter, which I want to look into anyway, but didnt want to do as a rush job.

esso23 commented 4 years ago

IStyled.CssAcync is just as fast ass IStyled.Css if I remove the await. So it's not about using async/sync methods, it's only about the blocking of OnAfterRenderAsync method on call to JS. I'd like to see the implementation of the "hiding until styles loaded" so that I can play around with it and maybe I'll be able to come up with something. I would appreciate if you could provide some code for that.

What I'm thinking could work is not blocking with await on the call to JS, but using ContinueWith() or some other form of "task completion subscription" to send a signal that the styles are loaded.

chanan commented 4 years ago

It's still a work on progress but I started working on this here: https://github.com/chanan/BlazorStyled/blob/master/src/SampleCore/Shared/MainLayout.razor#L176-L183