Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Blazor] Use JSON source generator during WebAssembly startup #54956
[Blazor] Use JSON source generator during WebAssembly startup #54956
Changes from all commits
a42088e
dfb6f93
11c518c
0eb1454
24335bc
54f2b82
361c91c
b06b3c4
17f9810
d758ef8
04910bb
75a71f6
35c4f33
03bdce3
fb95f5c
2996e5c
a10f5c0
48e7961
9a53261
f08d858
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another commit that eliminated this extra
JsonSerializerOptions
and serialized each argument individually, passing aJsonTypeInfo
for the argument directly and avoiding serializing anobject[]
. There appeared to be a perf benefit when running in the profiler, but it seemed to actually slightly degrade perf on optimized builds (but it was hard to tell), so I reverted it to just go with the simpler approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to avoid using the SG for this call, how big is the performance penalty?
This is a single call per app, and we are creating a bunch of interfaces to support it. Might not be worth the extra infrastructure just for this.
Alternatively, the renderer could expose a virtual
AttachWebRenderer
interop method that WebAssembly could override. If that were to be the case then we only expose a single method, not an extra interface. The differences would be that the method is likely less accessible and doesn't sit on the JSRuntime.Either way it's not a big deal IMO. If there is not a big penalty for avoiding this call through regular means, I would do that. If we benefit significantly, I will keep it and probably switch to use the virtual method instead of the extra interface, but its not a big deal eitherway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that the goal of this change is to improve startup costs, it shouldn't matter how many times a particular call is made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's about the trade-off of having an extra interface and coupling vs saving a couple MS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving a couple of MS sounds like a desirable goal assuming that it's a cost incurred by default for every wasm app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That couple-of-ms happens client-side as part of a several-hundred-ms startup process. It doesn't impact server CPU usage and hence mostly matters only to the extent that humans can notice it. So realistically it wouldn't be top of the queue of things for us to optimize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally, I just measured about an 80ms difference on published builds. Not sure exactly how much that difference changes on slower machines, but it seems significant. To put that number into perspective, total startup blocking time with that optimization is (locally) ~330ms.
We could, it would just mean that if we wanted to do a similar optimization in another area of the framework, we'd have to add additional specialized API (I count 9 other calls to
InvokeVoidAsync
inComponents.Web
, but those just happen to not occur during startup).Is it fine if we proceed with this for now and discuss it further in API review? I think it's an early enough preview where we could change the API later if needed, especially since it's annotated as
[EditorBrowsable(Never)]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about how you think we should maintain this in the future. How would a developer know when/what to add to this list of types? Am I right to think that if they start serializing something different and fail to add it here, everything would still work but would just be slower? Or would some error occur so they know to change this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along the same lines, how did you even know to include these specific types? I'm hoping it's because if you don't, there's a clear error indicating they need to be included!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types match the types of the arguments passed to the JS interop call. Failing to do this results in a runtime error. However, I've just pushed a change that makes it even clearer where these types come from, so hopefully that eliminates any confusion. If we end up reverting that change, I can add a comment explaining how these types should be specified.