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

v3 performance #104

Open
esso23 opened this issue Jun 17, 2020 · 13 comments
Open

v3 performance #104

esso23 opened this issue Jun 17, 2020 · 13 comments

Comments

@esso23
Copy link

esso23 commented Jun 17, 2020

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
Copy link
Owner

chanan commented Jun 17, 2020

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

@esso23
Copy link
Author

esso23 commented Jun 17, 2020

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:

await _scriptManager.UpdatedParsedClasses(_id.GetStableHashCodeString(), _id, _priority, parsedClasses);

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
Copy link
Owner

chanan commented Jun 17, 2020

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

@esso23
Copy link
Author

esso23 commented Jun 17, 2020

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
Copy link
Owner

chanan commented Jun 17, 2020

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
Copy link
Author

esso23 commented Jun 17, 2020

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
Copy link
Owner

chanan commented Jun 17, 2020

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
Copy link
Author

esso23 commented Jun 17, 2020

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
Copy link
Author

esso23 commented Jun 17, 2020

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
Copy link
Author

esso23 commented Jun 17, 2020

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

@chanan
Copy link
Owner

chanan commented Jun 17, 2020

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: #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
Copy link
Author

esso23 commented Jun 17, 2020

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
Copy link
Owner

chanan commented Jun 17, 2020

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

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

2 participants