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] Fix performance regression in STJ JsonNode instantiations #78147

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 10, 2022

Backport of #78130 to release/7.0

/cc @eiriktsarpalis @stephentoub

Customer Impact

Fixes a customer reported performance regression when creating JsonNode instances. This was caused by a static field in the class not having included the static keyword, resulting in expensive initializations on every JsonNode instantiation. According to benchmarks, this has resulted in said instantiation being 7x slower compared to .NET 6.

Testing

Added benchmarks to dotnet/performance measuring performance of JsonNode.

Risk

Low. Adds a missing static keyword to a readonly field declaration.

@ghost
Copy link

ghost commented Nov 10, 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

Backport of #78130 to release/7.0

/cc @eiriktsarpalis @stephentoub

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@teo-tsirpanis teo-tsirpanis added this to the 7.0.x milestone Nov 10, 2022
@eiriktsarpalis
Copy link
Member

@carlossanlop do we need to increment the servicing version for this change? It's already been incremented as part of #77388.

@eiriktsarpalis eiriktsarpalis self-assigned this Nov 10, 2022
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Nov 10, 2022
@eiriktsarpalis eiriktsarpalis changed the title [release/7.0] Fix expensive typo in JsonNode [release/7.0] Ensure JsonNode.s_defaultOptions field is static Nov 10, 2022
@eiriktsarpalis eiriktsarpalis changed the title [release/7.0] Ensure JsonNode.s_defaultOptions field is static [release/7.0] Fix performance regression in STJ JsonNode instantiations Nov 10, 2022
@ericstj
Copy link
Member

ericstj commented Nov 10, 2022

Is there any risk that this is now static? Are they mutable and would changes made by one instance now effect others when they didn't before?

@eiriktsarpalis
Copy link
Member

No such risk. The value is used as the default JsonSerializerOptions parameter in the JsonNode.WriteTo method -- it is passed to the built-in converters used when serializing JsonNode instances and is never passed to user-defined converters.

@ericstj
Copy link
Member

ericstj commented Nov 10, 2022

I see. Perhaps in main we can MakeReadonly to avoid any possible changes?

@eiriktsarpalis
Copy link
Member

Yes we could definitely make that change to avoid potential leaks of the value in the future.

@eiriktsarpalis eiriktsarpalis added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 11, 2022
@eiriktsarpalis
Copy link
Member

Servicing approved over email.

@carlossanlop
Copy link
Member

Now we only need a code review sign-off. @eiriktsarpalis or @layomia or @ericstj.

@carlossanlop
Copy link
Member

carlossanlop commented Nov 11, 2022

Approved by Tactics and signed-off by area owner.
The CI failure is known, it's a test project having trouble loading System.Text.Json correctly. But it's pre-existing and unrelated to this PR: #77988
This is an OOB package but we already merged another backport for System.Text.Json into release/7.0, so the OOB authoring changes needed are already merged to build it in 7.0.1:

<IsPackable>true</IsPackable>
<GeneratePackageOnBuild>true</GeneratePackageOnBuild>
<ServicingVersion>1</ServicingVersion>

Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 9fb0745 into release/7.0 Nov 11, 2022
@carlossanlop carlossanlop deleted the backport/pr-78130-to-release/7.0 branch November 11, 2022 17:29
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 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.

5 participants