Hot Reload agent improvements #60030
Conversation
* Hot Reload agent improvements Refactor agent code so that it can be shared with dotnet-watch implementation via a shared project. Add new API applyHotReloadDeltas to apply updates. * Feedback and cleanup
dotnet-issue-labeler
bot
added
the
area-blazor
|
@MackinnonBuck ptal |
src/Components/WebAssembly/WebAssembly/src/HotReload/MetadataUpdateHandlerInvoker.cs
Show resolved
Hide resolved
| @@ -1,29 +0,0 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
Do we have alternate test coverage ?
There was a problem hiding this comment.
I'm going to replace the agent code with a source package from https://github.com/dotnet/sdk/tree/main/src/BuiltInTools/HotReloadAgent in a follow up PR. dotnet/sdk has much more test coverage.
There was a problem hiding this comment.
Would that cover testing of WASM specifically ? There are lot of moving pieces to make it work in the browser.
There was a problem hiding this comment.
As far as I know there is no end-to-end test for WASM Hot Reload - i.e. testing that the change had an effect in the browser.
| if (response.IsSuccessStatusCode) | ||
| { | ||
| var deltasJson = await response.Content.ReadAsStringAsync(); | ||
| var updates = deltasJson != "" ? JsonSerializer.Deserialize<Update[]>(deltasJson, s_jsonSerializerOptions) : null; |
There was a problem hiding this comment.
Was the previous implementation also using JSON ?
Seems expensive for memory/cpu/transfer.
Could we make this AOT friendly by using code gen ?
There was a problem hiding this comment.
Or maybe the previous json parsing was in JS ? That would be cheaper.
There was a problem hiding this comment.
Previous parsing was in JS. Once we annotate this (dotnet/sdk#46369) it will be trimmed out.
There was a problem hiding this comment.
Could you please explain why/how this is better than the previous design ?
There was a problem hiding this comment.
I mean in the browser, anything that is C# will
- bring more IL code, larger download, slower startup.
- be slower, because we run interpreter to run managed code which parses the JSON
- the native
JSON.parsein the browser is very very fast in comparison.
Perhaps all of this could be neglected in the dev loop. Should it be ?
Could we rather transfer binary instead of JSON ?
There was a problem hiding this comment.
Ok, fine. I would still appreciate some description of what's the overall plan.
Also I'm worried that changes in the SDK will have impact on the previous versions of the product.
There was a problem hiding this comment.
@pavelsavara See #60030 (comment) for context.
We prioritize simplicity (avoiding versioning nightmares) over performance here since Hot Reload is development-only tool. Moving the code to C# avoids calls between SDK code and WASM code, which version differently. That said we obviously don't want to regress WASM app size in production scenario (trimmed). Applying proper trimming attributes is definitely fair ask and we need to follow up on that (dotnet/sdk#46369).
That said, since the size of WASM app is important shouldn't there be an existing test that validates some baseline set of references and expected size range for a simple app? I'd expect such test to exist and fail in the 9.0 branch where these changes were already made a while ago, if in fact using C# JSON deserializer has significant impact. Not saying it does not but we need to start with definition of what is significant and keep measuring it in CI so that we don't regress.
Is the impact significant enough to warrant servicing 9.0? As I mentioned this exact code already shipped in 9.0 and I have not seen any test failing that would indicate we regressed size of WASM apps.
There was a problem hiding this comment.
The size of the app fluctuates with every PR. There is regular pref/size regression triage.
I'm not sure we triage it for released branches. I'm not super happy that architectural change landed as patch.
Anyway, it's easier to write test for trimming specific class, than automating size trend detection and triage.
My 2c
There was a problem hiding this comment.
Hot Reload types themselves are rather small. I don't think they cause significant size increase even if they are not trimmed at all.
What I'd be more worried about is usage of JSON. That should be easy to validate by checking if there are any types that originated in references that are not expected to be there, or checking namespaces of the types.
There was a problem hiding this comment.
Test with reflection asserting that class WebAssemblyHotReload can't be found will do the trick.
|
@javiercn ptal |
There was a problem hiding this comment.
@tmat I'm assuming this is a forward port of the PR you referenced.
I'm not super familiar with this area of the code, so I think a bit of context on how this used to work and what's changing and why would be helpful in the review process.
|
@javiercn Hot Reload system is rather complex, not sure how much detail would you like to dive into.
WASM Hot Reload is more complex than .NET app Hot Reload because in the latter case the IDE/dotnet-watch injects agent to the target process while in WASM we can't do that. The agent in WASM needs to be built-in. Because the WASM runtime is brought into the app via nuget package, it versions differently than the IDE/dotnet-watch. Therefore, any communication between the agent and the IDE/dotnet-watch needs multiple combinations of versions (e.g. WASM 8.0 with SDK 9.0.200). This makes it difficult to make any changes to the protocol. Some of the changes in this PR (moving code from JavaScript to C#) aim to reduce the amount of APIs/endpoints that cross this versioning boundary. |
|
I have many questions, mostly because I was not part of hot-reload before.
By protocol, you mean the shape of "delta" bytes, right ? Is this PR or dotnet/sdk#44539 actually making protocol changes ? If not, what is the last PR which made any changes to it ? Which flow (repository) is responsible for version of that protocol runtime/SDK/aspnecore/VS/another ? Do the recent changes support backward and forward compatibility of VS/SDK/runtime ? For example, can old VS/SDK still talk to project with new runtime+blazor on protcol level ? Could you please explain versioning dependency between VS and the rest of it in more detail ?
This is more or less transport level change, right ? Does it break the transport from new IDE/dotnet-watch to old WASM projects ? Can latest IDE/dotnet-watch transport to Net8 WASM app ? (Net8 is Long Term Support) @javiercn why is hot reload Blazor WASM feature and not WASM runtime feature ? (non-blazor WASM app template) |
Yes, but also endpoints that are invoked on the server side and served by Hot Reload middleware and calls between WASM ASP.NET code and JS scripts included in the SDK.
Both are - one for each end of the communication channel. More precisely, this one was the WASM change in 9.0: #58333
dotnet/sdk.
Yes.
VS currently has it's own implementation of Hot Reload components, but I'm working on unifying it with dotnet-watch.
Yes. |
What makes the 44539 to not break the Net8 compatibility ? Because SDK is shared across versions and will get updated to latest on customer dev machines. |
We still have the old code path around. It just doesn't support the added features (logging). |
Ports #58333 to main.
Refactor agent code so that it can be shared with dotnet-watch implementation via a shared project.
Add new API applyHotReloadDeltas to apply updates.