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/6.0] Remove redundant allocations from JsonSerializerOptions.GetConverter calls #65898

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Feb 25, 2022

Fixes Issue #65770, cherry-picks the fix from #65863.

Customer Impact

Fixes a customer reported issue. Currently every call to JsonSerializerOptions.GetConverter will force recreation of the default converters global state, which can substantially impact the allocation profile for custom converters that dynamically look up other converters. This is a regression from .NET 5.

Diff without whitespace

Testing

N/A

Risk

Low. Brings back a trivial check in the global state initialization method.

@ghost
Copy link

ghost commented Feb 25, 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

Fixes Issue #65770, cherry-picks the fix from #65863.

Customer Impact

Fixes a customer reported issue. Currently every call to JsonSerializerOptions.GetConverter will force recreation the default converters global state, which can substantially impact the allocation profile for custom converters that dynamically look up other converters. This is a regression from .NET 5.

Testing

N/A

Risk

Low. Brings back a trivial check in the global state initialization method.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis requested a review from krwq February 25, 2022 16:48
@eiriktsarpalis eiriktsarpalis added this to the 6.0.x milestone Feb 25, 2022
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Feb 25, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 1, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.4 Mar 1, 2022
@carlossanlop
Copy link
Member

@eiriktsarpalis @steveharter @layomia just noticed this change affects an OOB package. Per the servicing instructions, if the csproj has the <IsPackable>true</IsPackable> property (which it does), then you also need to set the <GeneratePackageOnBuild>true</GeneratePackageOnBuild> property, and bump the <ServicingVersion>2</ServicingVersion> property to the next integer (3).

Here are a couple of examples (we had to add the property on the next release because we forgot): #65523 #65733

Can one of you please add the change?

@eiriktsarpalis
Copy link
Member Author

@carlossanlop done.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package change looks good.

@ericstj
Copy link
Member

ericstj commented Mar 10, 2022

AOT failure is addressed in #66366. Linux_musl test failure is #56087

@ericstj ericstj merged commit 3dd25be into release/6.0 Mar 10, 2022
@eiriktsarpalis eiriktsarpalis deleted the backport/fix-getconverter-allocations branch March 10, 2022 20:58
@layomia layomia changed the title Remove redundant allocations from JsonSerializerOptions.GetConverter calls [release/6.0] Remove redundant allocations from JsonSerializerOptions.GetConverter calls Mar 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 13, 2022
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.

6 participants