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

Blazor Wasm - Size regression in System.Text.Json.dll #51311

Closed
SamMonoRT opened this issue Apr 15, 2021 · 12 comments
Closed

Blazor Wasm - Size regression in System.Text.Json.dll #51311

SamMonoRT opened this issue Apr 15, 2021 · 12 comments
Labels
arch-wasm WebAssembly architecture area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@SamMonoRT
Copy link
Member

SamMonoRT commented Apr 15, 2021

System.Text.Json.dll.br has a big spike in last two days as observed in https://msit.powerbi.com/groups/me/apps/54e0e83f-07bc-45bf-87b7-a7677ff3af2a/dashboards/fa051820-ff60-4d40-8a08-bdcc1b47b1d0

image

@eerhardt @CoffeeFlux

@SamMonoRT SamMonoRT added arch-wasm WebAssembly architecture size-reduction Issues impacting final app size primary for size sensitive workloads labels Apr 15, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Apr 15, 2021
@ghost
Copy link

ghost commented Apr 15, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

System.Json.Text.dll,br has a big spike in last two days as observed in https://msit.powerbi.com/groups/me/apps/54e0e83f-07bc-45bf-87b7-a7677ff3af2a/dashboards/fa051820-ff60-4d40-8a08-bdcc1b47b1d0

image

@eerhardt @CoffeeFlux

Author: SamMonoRT
Assignees: -
Labels:

arch-wasm, size-reduction

Milestone: -

@SamMonoRT SamMonoRT changed the title Blazor Wasm - Size regression in System.Json.Text.dll Blazor Wasm - Size regression in System.Text.Json.dll Apr 15, 2021
@eerhardt
Copy link
Member

@eerhardt suggested that these 2 changes are possible offenders

#50778
#51025

@layomia @steveharter @eiriktsarpalis

@layomia
Copy link
Contributor

layomia commented Apr 16, 2021

These two changes are primarily JsonConverter-based features and are very likely to have caused the regression.

The JSON source generator will help reduce size for apps that use the new overloads that take pre-generated metadata (#51300). The converters for these features will be trimmed out if not needed in an app, and if the new serializer methods are used instead of the existing ones.

I don't expect the size regression here to be reversed until the JSON serialization logic within Blazor assemblies is refactored to use the new serializer methods. See these commits for an early attempt. Happy to help with this effort.

@SamMonoRT
Copy link
Member Author

Thanks @layomia - do we know if there is any tracking issue for the refactoring work ?

@SamMonoRT
Copy link
Member Author

cc @mkArtakMSFT - is this on your team ?

@SamMonoRT
Copy link
Member Author

SamMonoRT commented Apr 16, 2021

Want to put this note out, to make it very clear - We don't expect any changes from dotnet/aspnetcore#31877 to be backported to Preview 4. But we need to make sure the necessary work has a timeline to make it in quickly.

@steveharter
Copy link
Member

steveharter commented Apr 21, 2021

One potential path to reduce size as new features (and converters) are added is to initialize JsonSerializerOptions with no converters and then add only the converters necessary for the given application and scenarios. @layomia recently added the source-gen \ trimming work that does this but it could be extended and used without source-gen:

internal class MyCustomContext : JsonSerializerContext
{
    public MyCustomContext(JsonSerializerOptions options) : base(options) { }
    // Also possible to cache\expose the JsonTypeInfos here...
}

JsonSerializerOptions options = new JsonSerializerOptions;
options.Converters.Add(JsonConverters.String);
options.Converters.Add(JsonConverters.Int32);
options.Converters.Add(JsonConverters.JsonNodeConverterFactory);
// etc

var context = new MyCustomContext(options);
JsonNode node = ...
JsonSerializer.Serialize(node, context);

@SamMonoRT
Copy link
Member Author

Assigning to @layomia for tracking based on additional work building over the JSON SourceGenerator work

@layomia
Copy link
Contributor

layomia commented May 17, 2021

Following up on offline discussions on email, I'm noting on this issue that the path to reducing the regression here will not be about Blazor fully adopting the JSON source generator internally and in the default template (dotnet/aspnetcore#31877). Size is important for Blazor, but it was discussed that the generator should focus on trim safety (already supported), and improve (de)serialization throughput (in progress with #51945).

For mitigating the size regression here, this issue will instead focus on internal System.Text.Json changes to reduce the assembly size when source gen is not used:

cc @SamMonoRT @CoffeeFlux @eerhardt @steveharter @ericstj @SteveSandersonMS @pranavkm @jkotas

@lewing
Copy link
Member

lewing commented Jul 31, 2021

Is there any remaining .NET6 work here?

@layomia
Copy link
Contributor

layomia commented Aug 9, 2021

I believe there's no work left here for 6.0 given the above plan #51311 (comment). cc @eerhardt

@layomia layomia modified the milestones: 6.0.0, 7.0.0 Aug 9, 2021
@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Jul 10, 2022
@eiriktsarpalis
Copy link
Member

I think we can close this issue now?

@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

No branches or pull requests

7 participants