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

Consider caching Utf8JsonWriter and PooledByteBufferWriter on a thread static in JsonSerializer.Serialize #69889

Closed
Tracked by #63918
davidfowl opened this issue May 27, 2022 · 7 comments · Fixed by #73338
Assignees
Labels
Milestone

Comments

@davidfowl
Copy link
Member

Description

Looking into the allocation profile for our JSON tech empower benchmark, the allocation overhead for the JsonSerializer itself

Configuration

Run this crank command.

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/json.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/azure.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario https --profile intel-lin-app --profile intel-load2-load --application.framework net7.0 --application.collectDependencies true --application.options.collectCounters true --application.aspNetCoreVersion 7.0.0-preview.6.22275.2 --application.runtimeVersion 7.0.0-preview.6.22276.3 --application.sdkVersion 7.0.100-preview.6.22275.1  --application.dotnetTrace true --application.dotnetTraceProviders gc-verbose

This will run the JsonHttps benchmark and it'll spit out an allocation profile that you can view in perfview (alloc tick events).

Regression?

No it's not

Data

Name                                         Exc %           Exc Exc CtInc %             Inc Inc CtFoldFold Ct                             When      First       Last
Type System.Text.Json.Utf8JsonWriter          73.52,195,098,368.000 20,668 73.52,195,098,368.000 20,668   0      0 79oEEEDEEDEEEEEDCEEEFEDFEDEEDEED 21,998.809 52,258.721
Type System.Text.Json.PooledByteBufferWriter 16.5   493,749,376  4,649 16.5     493,749,376  4,649   0      0 112o3332333332333333233333333333 22,031.344 52,235.635
Type Benchmarks.Middleware.JsonMessage         9.5   284,879,104  2,682  9.5     284,879,104  2,682   0      0 00110121121112111111112112222111 22,016.234 52,217.968

Analysis

We have more infrastructure allocation overhead than we do serializing the actual object.

@davidfowl davidfowl added the tenet-performance Performance related issue label May 27, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 27, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented May 27, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Looking into the allocation profile for our JSON tech empower benchmark, the allocation overhead for the JsonSerializer itself

Configuration

Run this crank command.

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/json.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/azure.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario https --profile intel-lin-app --profile intel-load2-load --application.framework net7.0 --application.collectDependencies true --application.options.collectCounters true --application.aspNetCoreVersion 7.0.0-preview.6.22275.2 --application.runtimeVersion 7.0.0-preview.6.22276.3 --application.sdkVersion 7.0.100-preview.6.22275.1  --application.dotnetTrace true --application.dotnetTraceProviders gc-verbose

This will run the JsonHttps benchmark and it'll spit out an allocation profile that you can view in perfview (alloc tick events).

Regression?

No it's not

Data

Name                                         Exc %           Exc Exc CtInc %             Inc Inc CtFoldFold Ct                             When      First       Last
Type System.Text.Json.Utf8JsonWriter          73.52,195,098,368.000 20,668 73.52,195,098,368.000 20,668   0      0 79oEEEDEEDEEEEEDCEEEFEDFEDEEDEED 21,998.809 52,258.721
Type System.Text.Json.PooledByteBufferWriter 16.5   493,749,376  4,649 16.5     493,749,376  4,649   0      0 112o3332333332333333233333333333 22,031.344 52,235.635
Type Benchmarks.Middleware.JsonMessage         9.5   284,879,104  2,682  9.5     284,879,104  2,682   0      0 00110121121112111111112112222111 22,016.234 52,217.968

Analysis

We have more infrastructure allocation overhead than we do serializing the actual object.

Author: davidfowl
Assignees: -
Labels:

area-System.Text.Json, tenet-performance, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label May 31, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone May 31, 2022
@eiriktsarpalis eiriktsarpalis self-assigned this Jul 19, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 3, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 5, 2022
@davidfowl
Copy link
Member Author

I'm running this performance test after this change and I'm not seeing a big difference 🤔.

Command:

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/json.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/azure.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario https --profile intel-lin-app --profile intel-load2-load --application.framework net7.0 --application.collectDependencies true --application.options.collectCounters true --application.aspNetCoreVersion 7.0.0-rc.1.22413.3 --application.runtimeVersion 7.0.0-rc.1.22413.9 --application.sdkVersion 7.0.100-rc.1.22413.1 --application.dotnetTrace true --application.dotnetTraceProviders gc-verbose
Name                                         Exc %           Exc Exc CtInc %             Inc Inc CtFoldFold Ct                             When     First       Last
Type System.Text.Json.Utf8JsonWriter          72.82,274,646,784.000 21,417 72.82,274,646,784.000 21,417   0      0 ___40EDEEDEDDEEEECEEEFEEEDEEDEED 3,523.967 33,786.321
Type System.Text.Json.PooledByteBufferWriter 16.9   529,322,496  4,984 16.9     529,322,496  4,984   0      0 ___12o33333333333233333333333332 3,547.375 33,767.268
Type Benchmarks.Middleware.JsonMessage         9.6   300,157,952  2,826  9.6     300,157,952  2,826   0      0 ___01o21221122211111211111112221 3,553.376 33,764.016
Type System.Byte[]                             0.2     5,190,632     48  0.2       5,297,120     49   0      0 o__o0o__o________0______________     7.488 18,760.149

cc @sebastienros

@eiriktsarpalis
Copy link
Member

Could it be that the benchmark uses one of the async methods? No attempt to cache is being made there.

https://github.com/aspnet/Benchmarks/blob/62f41b881092696b9775529ba2f825e96764b4ea/src/Benchmarks/Middleware/JsonMiddleware.cs#L56

@davidfowl
Copy link
Member Author

davidfowl commented Aug 17, 2022

Why not? I missed that in the PR... (well I know why its not thread local for that case 😄)..

@eiriktsarpalis
Copy link
Member

I figured it might not be as important given the added overhead of state machines. The benefits in the sync case seem fairly modest and only register for trivially sized object graphs (i.e., those of depth < 2)

@davidfowl
Copy link
Member Author

davidfowl commented Aug 18, 2022

That's pretty reasonable. I think in the benchmark that could be synchronous because the object is tiny and will fit in the buffer. It's more problematic in general for 2 reasons:

  • When the code doing the serialization doesn't know what object is being serialized or you can't really eye ball the size to know if it'll fit into the buffer.
  • Sync + ASP.NET Core = ☠️

@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants