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

[release/7.0] Remove ConcurrentDictionary from the shared JsonSerializerOptions cache implementation. #80576

Merged

Conversation

eiriktsarpalis
Copy link
Member

Fixes #80258

Backport of #76607, #76782

Customer Impact

.NET 7 saw the release of a global JsonSerializerOptions metadata cache, intended to improve performance for users that create transient JsonSerializerOptions instances in their serialization operations. The implementation uses a ConcurrentDictionary keyed on JsonSerializerOptions configuration points, pointing to weak references of metadata caches. The cache uses a manual eviction strategy that removes dangling dictionary entries, triggered on every deserialization operation.

What this approach failed to foresee is that JsonSerializerOptions configuration points can be fairly large in their own right. In particular, Blazor uses transient JsonSerializerOptions instances as a vehicle for performing dependency injection in their custom converters. As a result, the custom converters that Blazor uses capture a substantial amount of memory that gets rooted by the global cache. Even though these keys will eventually get evicted, when this happens is predicated on the frequency of subsequent serialization operations, so in certain cases this can trigger bounded memory leaks.

The fix involves removing the ConcurrentDictionary and replacing it with a fixed-size array of weak references that are looked up using linear traversal. This removes the need for manual eviction and simplifies the cache implementation. Even though this was fixed in .NET 8 back in October, we've received multiple Blazor customer reports about this issue, convincing us it should be backported to .NET 7.

Testing

Manually validated that memory leaks no longer occur in reproducing Blazor apps. Ran benchmarks confirming that global cache performance hasn't regressed.

Risk

Moderate. It is an involved rewrite of a cache implementation, but it has been present in the .NET 8 branch for the past 3 months.

@ghost
Copy link

ghost commented Jan 12, 2023

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

Fixes #80258

Backport of #76607, #76782

Customer Impact

.NET 7 saw the release of a global JsonSerializerOptions metadata cache, intended to improve performance for users that create transient JsonSerializerOptions instances in their serialization operations. The implementation uses a ConcurrentDictionary keyed on JsonSerializerOptions configuration points, pointing to weak references of metadata caches. The cache uses a manual eviction strategy that removes dangling dictionary entries, triggered on every deserialization operation.

What this approach failed to foresee is that JsonSerializerOptions configuration points can be fairly large in their own right. In particular, Blazor uses transient JsonSerializerOptions instances as a vehicle for performing dependency injection in their custom converters. As a result, the custom converters that Blazor uses capture a substantial amount of memory that gets rooted by the global cache. Even though these keys will eventually get evicted, when this happens is predicated on the frequency of subsequent serialization operations, so in certain cases this can trigger bounded memory leaks.

The fix involves removing the ConcurrentDictionary and replacing it with a fixed-size array of weak references that are looked up using linear traversal. This removes the need for manual eviction and simplifies the cache implementation. Even though this was fixed in .NET 8 back in October, we've received multiple Blazor customer reports about this issue, convincing us it should be backported to .NET 7.

Testing

Manually validated that memory leaks no longer occur in reproducing Blazor apps. Ran benchmarks confirming that global cache performance hasn't regressed.

Risk

Moderate. It is an involved rewrite of a cache implementation, but it has been present in the .NET 8 branch for the past 3 months.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Jan 12, 2023
@eiriktsarpalis eiriktsarpalis changed the title Remove ConcurrentDictionary from the shared JsonSerializerOptions cache implementation. [release/7.0] Remove ConcurrentDictionary from the shared JsonSerializerOptions cache implementation. Jan 12, 2023
@carlossanlop
Copy link
Member

For clarity, this PR does not need OOB changes added because it will go in the same release as this other PR for the same assembly, which already includes the required OOB changes: https://github.com/dotnet/runtime/pull/80513/files

@carlossanlop
Copy link
Member

@eiriktsarpalis can you please solve the merge conflict? It showed up after merging the other Json PR.

@eiriktsarpalis
Copy link
Member Author

Changes rebased.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 13, 2023
@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.3 Jan 13, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email (7.0.3).
CI failure is a networking issue that I've seen in other backports since yesterday: #80578
The required OOB changes are already in (see #80576 (comment)).
Need a code review sign-off from an area owner. @layomia / @krwq?

@carlossanlop carlossanlop merged commit 5359311 into dotnet:release/7.0 Jan 13, 2023
@eiriktsarpalis eiriktsarpalis deleted the backport-options-cache branch January 26, 2023 11:53
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants