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

System.Text.Json.JsonProperty allocated a string in WriteTo() #88767

Closed
mwadams opened this issue Jul 12, 2023 · 3 comments · Fixed by #90074
Closed

System.Text.Json.JsonProperty allocated a string in WriteTo() #88767

mwadams opened this issue Jul 12, 2023 · 3 comments · Fixed by #90074
Labels
area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone
Future

Comments

@mwadams
Copy link
Contributor

mwadams commented Jul 12, 2023

Description

System.Text.Json.JsonProperty.WriteTo() uses the Name property to write name and value.

This realises the property name as a string, causing an allocation.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonProperty.cs

Line 120 in 0a50fef

writer.WritePropertyName(Name);

It would be preferable to write the property name using the underlying JsonElement and avoid the allocation.
@mwadams mwadams added the tenet-performance Performance related issue label Jul 12, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 12, 2023
@ghost
Copy link

ghost commented Jul 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

Description

System.Text.Json.JsonProperty.WriteTo() uses the Name property to write name and value.

This realises the property name as a string, causing an allocation.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonProperty.cs

Line 120 in 0a50fef

writer.WritePropertyName(Name);

It would be preferable to write the property name using the underlying JsonElement and avoid the allocation.

Author: mwadams
Assignees: -
Labels:

area-System.Text.Json, tenet-performance
Milestone: -

@eiriktsarpalis
Copy link
Member

Thanks for reporting this. Would you be interested in contributing a PR that fixes the issue?

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jul 13, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Jul 13, 2023
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Jul 13, 2023
@karakasa
Copy link
Contributor

karakasa commented Aug 5, 2023

i would try to implement a fix.

karakasa added a commit to karakasa/runtime that referenced this issue Aug 5, 2023
@karakasa
Avoid string allocation in WriteTo when possible.
ad30f54 
System.Text.Json.JsonProperty.WriteTo uses get_Name, calling
JsonElement.GetPropertyName() which would allocate a string.

Use ReadOnlySpan<byte> from the underlying UTF8 json, when possible,
by adding helper methods into JsonDocument & JsonElement.

Fix dotnet#88767
@karakasa karakasa mentioned this issue Aug 6, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 6, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 10, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 21, 2023
eiriktsarpalis pushed a commit that referenced this issue Sep 22, 2023
@karakasa
Avoid string allocation and improve performance of `JsonProperty.Writ…
45fe624 
…eTo` (#90074)

* Avoid string allocation in WriteTo when possible.

System.Text.Json.JsonProperty.WriteTo uses get_Name, calling
JsonElement.GetPropertyName() which would allocate a string.

Use ReadOnlySpan<byte> from the underlying UTF8 json, when possible,
by adding helper methods into JsonDocument & JsonElement.

Fix #88767

* Avoid alloc in unescaping & escaping.

Current code unescapes & escapes property names and uses ToArray.
Avoid alloc by adding internal GetRaw/WriteRaw methods.

* Fix bugs on escaped property names

Original code doesn't handle GetRaw/WriteRaw on escaped
property names correctly.

* Change IndexOf to Contains if possible.

* Further avoid alloc by inlining GetUnescapedSpan

Allocations are further avoided when the property name is shorter than
JsonConstants.StackallocByteThreshold, by inlining
JsonReaderHelper.GetUnescapedSpan.

* Move writing logic to JsonElement;
Shorten names of new methods;
Add a test of writing out special names.

* Move logic into JsonDocument

* fix format

* fix format 2

* improve comment

* removed unused stub

* added assertion

* removed unnecessary test
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
Milestone
Future
Development

Successfully merging a pull request may close this issue.

Avoid string allocation and improve performance of JsonProperty.WriteTo karakasa/runtime
3 participants
@mwadams @eiriktsarpalis @karakasa