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.

Sign up for GitHub

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

Jump to bottom

Use global caching in JsonSerializerOptions #64646

Merged
merged 10 commits into from
Feb 14, 2022
Merged

Use global caching in JsonSerializerOptions #64646

merged 10 commits into from
Feb 14, 2022

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Feb 1, 2022 •
edited
Loading

Applies global caching for reflection metadata in two separate layers:

  1. At the MethodAccessor layer caching generated dynamic methods.
  2. Refactors the JsonSerializerOptions caching, using shared caching contexts shared across multiple JsonSerializerOptions instances "up to equivalence".

Contributes to #40072.

Performance

Using benchmarks from dotnet/performance#2236

Method Job Toolchain Mean Error StdDev Median Min Max Ratio MannWhitney(3%) RatioSD Gen 0 Gen 1 Gen 2 Allocated Alloc Ratio
CachedDefaultOptions Job-IDHNJC main 742.3 ns 55.64 ns 64.08 ns 730.1 ns 618.9 ns 837.6 ns 1.00 Base 0.00 0.0317 - - 320 B 1.00
CachedDefaultOptions Job-VHZIGZ PR 779.1 ns 40.98 ns 47.20 ns 783.2 ns 698.4 ns 850.5 ns 1.06 Same 0.11 0.0292 - - 320 B 1.00
NewDefaultOptions Job-IDHNJC main 462,649.4 ns 29,400.08 ns 33,857.17 ns 466,119.9 ns 378,260.7 ns 508,198.9 ns 1.000 Base 0.00 - - - 13234 B 1.00
NewDefaultOptions Job-VHZIGZ PR 1,281.0 ns 76.38 ns 87.96 ns 1,285.5 ns 1,109.7 ns 1,449.0 ns 0.003 Faster 0.00 0.0586 0.0098 0.0049 637 B 0.05
NewCustomizedOptions Job-IDHNJC main 450,203.9 ns 26,126.89 ns 29,039.97 ns 453,785.3 ns 408,028.4 ns 502,898.1 ns 1.000 Base 0.00 - - - 13317 B 1.00
NewCustomizedOptions Job-VHZIGZ PR 1,402.5 ns 85.25 ns 98.18 ns 1,383.5 ns 1,245.4 ns 1,577.6 ns 0.003 Faster 0.00 0.0631 0.0105 0.0053 662 B 0.05
NewCustomConverter Job-IDHNJC main 388,271.0 ns 18,375.55 ns 19,661.63 ns 387,544.6 ns 348,863.4 ns 426,233.6 ns 1.00 Base 0.00 - - - 13356 B 1.00
NewCustomConverter Job-VHZIGZ PR 17,386.3 ns 970.16 ns 1,078.34 ns 17,761.1 ns 15,454.0 ns 19,390.9 ns 0.04 Faster 0.00 0.5618 0.0702 - 6098 B 0.46
NewCachedCustomConverter Job-IDHNJC main 450,729.7 ns 30,334.93 ns 34,933.76 ns 451,971.2 ns 383,972.0 ns 517,959.8 ns 1.000 Base 0.00 - - - 13319 B 1.00
NewCachedCustomConverter Job-VHZIGZ PR 1,461.3 ns 93.35 ns 107.50 ns 1,455.3 ns 1,240.6 ns 1,659.3 ns 0.003 Faster 0.00 0.0696 0.0107 0.0054 719 B 0.05

@eiriktsarpalis eiriktsarpalis self-assigned this Feb 1, 2022
@ghost
Copy link

ghost commented Feb 1, 2022

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

Issue Details

Work in progress.

  • Use global caching in ReflectionEmitMethodAccessor.
  • Reuse converter caches across "equivalent" JsonSerializerOptions instances.

Contributes to #40072.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json
Milestone: -

@eiriktsarpalis eiriktsarpalis marked this pull request as ready for review February 2, 2022 23:11
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Feb 2, 2022
@eiriktsarpalis eiriktsarpalis added the tenet-performance Performance related issue label Feb 2, 2022
@eiriktsarpalis eiriktsarpalis force-pushed the member-accessor-caching branch from 61114a0 to 4456cf1 Compare February 4, 2022 19:06
@eiriktsarpalis eiriktsarpalis marked this pull request as draft February 4, 2022 19:07
@eiriktsarpalis eiriktsarpalis force-pushed the member-accessor-caching branch from 4456cf1 to 8050aad Compare February 4, 2022 19:18
@eiriktsarpalis eiriktsarpalis force-pushed the member-accessor-caching branch from 8050aad to 295796f Compare February 4, 2022 22:34
@eiriktsarpalis eiriktsarpalis marked this pull request as ready for review February 4, 2022 22:59
layomia
layomia reviewed Feb 9, 2022
View reviewed changes
...Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs Outdated Show resolved Hide resolved
...son/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.Cache.cs Outdated Show resolved Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs Show resolved Hide resolved
...System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs Show resolved Hide resolved
...son/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.Cache.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Outdated Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Outdated Show resolved Hide resolved
@eiriktsarpalis eiriktsarpalis force-pushed the member-accessor-caching branch from d0ad2ef to 0e049e7 Compare February 10, 2022 13:04
@eiriktsarpalis eiriktsarpalis mentioned this pull request Feb 14, 2022
Open
@eiriktsarpalis eiriktsarpalis merged commit 8e4bef2 into dotnet:main Feb 14, 2022
@eiriktsarpalis eiriktsarpalis deleted the member-accessor-caching branch February 14, 2022 23:46
stephentoub
stephentoub reviewed Feb 15, 2022
View reviewed changes
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs Show resolved Hide resolved
@eiriktsarpalis eiriktsarpalis mentioned this pull request Feb 15, 2022
Merged
eiriktsarpalis added a commit to eiriktsarpalis/runtime that referenced this pull request Feb 15, 2022
@eiriktsarpalis
Incorporate review feedback from
6a4d140 
dotnet#64646
This was referenced Feb 15, 2022
Closed
Closed
eiriktsarpalis added a commit that referenced this pull request Feb 15, 2022
@eiriktsarpalis
Incorporate review feedback from #64646 (#65376)
6b3ec8e 
* Incorporate review feedback from
#64646

* Prevent trimming of unused CachingContext.Count method

* use correct syntax for nested types
@kunalspathak
Copy link
Member

kunalspathak commented Feb 22, 2022 •
edited
Loading

Improvements: dotnet/perf-autofiling-issues#3641, dotnet/perf-autofiling-issues#3649, dotnet/perf-autofiling-issues#3656, dotnet/perf-autofiling-issues#3663, dotnet/perf-autofiling-issues#3671

This was referenced Feb 23, 2022
Closed
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vitek-karas vitek-karas vitek-karas left review comments

@eerhardt eerhardt eerhardt left review comments

@stephentoub stephentoub stephentoub left review comments

@layomia layomia layomia approved these changes

@steveharter steveharter Awaiting requested review from steveharter

Assignees

@eiriktsarpalis eiriktsarpalis

Labels
area-System.Text.Json tenet-performance Performance related issue
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@eiriktsarpalis @kunalspathak @stephentoub @eerhardt @layomia @vitek-karas