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

Normalizing casing for property names in JsonException.Path helps deserialization performance #30789

Closed
steveharter opened this issue Sep 6, 2019 · 2 comments · Fixed by #35848
Assignees
Labels
area-System.Text.Json design-discussion Ongoing discussion about design without consensus tenet-performance Performance related issue
Milestone

Comments

@steveharter
Copy link
Member

Creating this issue to discuss feasibility of getting this into 5.0. cc @pranavkm @ahsonkhan @rynowak

If we remove support for using the literal JSON value for JsonException.Path we can get a ~5% end-to-end performance improvement during deserialization when using options.CaseInsensitivePropertyNames=true (which is the default for ASP.NET). Allocations are also reduced by ~30% (depends on property name lengths).

UPDATE: recent measurements for a very simple case show up to 30% improvement, not 5%. Todo: need to quantify this here.

This means that when case insensitivity is on, the value of the JsonException.Path property may not exactly match the property name casing in the JSON (the value is always correct; just the casing can be off).

There are three ways the JSON property name is specified:

  1. The object's CLR property reflected name (default case).
  2. The value from [JsonPropertyName] attribute applied to a property.
  3. The naming policy from options.PropertyNamingPolicy such as camel-casing.

So currently if there exists JSON like {"myProp:1"} against a property named MyProp (either through case 1, 2 or 3 above) then when case-insensitivity is on the JsonException.Path will be "$.myProp".

However, using the raw JSON value comes at a cost for case insensitivity. Instead if we just use the value obtained from case 1, 2 or 3 (and not the actual JSON) then we get the perf improvement -- e.g. path would be "$.MyProp" instead of {"myProp:1"} (again this would only occur when case insensitivity is on and does not match the actual property name).

Current:
|                   Method |     Mean |    Error |   StdDev |   Median |      Min |      Max | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|------------------------- |---------:|---------:|---------:|---------:|---------:|---------:|------------:|------------:|------------:|--------------------:|
|    DeserializeFromString | 662.8 ns | 3.631 ns | 3.218 ns | 662.8 ns | 658.7 ns | 670.2 ns |      0.0611 |           - |           - |               384 B |
| DeserializeFromUtf8Bytes | 614.0 ns | 2.882 ns | 2.696 ns | 614.6 ns | 609.1 ns | 618.9 ns |      0.0420 |           - |           - |               272 B |
|    DeserializeFromStream | 915.3 ns | 3.769 ns | 3.341 ns | 916.6 ns | 908.8 ns | 918.4 ns |      0.0513 |           - |           - |               344 B |

After:
|    DeserializeFromString | 637.5 ns | 3.702 ns | 3.463 ns | 636.6 ns | 632.2 ns | 643.7 ns |      0.0436 |           - |           - |               280 B |
| DeserializeFromUtf8Bytes | 586.1 ns | 4.028 ns | 3.768 ns | 585.3 ns | 579.7 ns | 592.6 ns |      0.0259 |           - |           - |               168 B |
|    DeserializeFromStream | 913.7 ns | 4.604 ns | 4.307 ns | 912.7 ns | 907.1 ns | 921.5 ns |      0.0369 |           - |           - |               240 B |
@steveharter steveharter self-assigned this Sep 6, 2019
@steveharter
Copy link
Member Author

An alternative to normalizing the casing (and deal with the con of "incorrect" casing on JsonPath) is to extend the property lookup caching to say whether the current JSON property matches a cached property name value. This would avoid an alloc and be fast if the JSON is consistent meaning different payloads use the exact casing (otherwise it will fall back to slower path).

@steveharter
Copy link
Member Author

This was addressed by the commit #35848 by not normalizing -- instead there is a comparison of the property name in JSON against the cached data and an alloc is only created when necessary. When the alloc is necessary, it is cached if the 64-element array-based cache is not full yet.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json design-discussion Ongoing discussion about design without consensus tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants