Port perf changes and issues #41771

steveharter · 2019-10-14T20:01:09Z

The "No Merge" label is there to ensure when this is merged,the individual commits are NOT squashed (one commit here per original commit in master).

Peformance and three bug fixes:
Fixes #40449
Fixes #40704
Fixes #41425

On performance:
• Serialization is ~1.2x to ~1.5x faster than 3.0
• Deserialization is ~1.2x faster than 3.0

Ports the following:

Issue	Type	5.0 PR	5.0 Commit	Description	Customer Impact	Risk
40449	Bug	40501	`5dbbdfa`	Change a NullReferenceException to JsonException when invalid JSON is encountered in certain cases.	Reported by the community. Invalid JSON can result in a NullReferenceException which is not expected and likely not in any Try\Catch.	Low. The fix has been in master for several weeks. Not likely to be “breaking” since changing from NullReferenceException to another (valid) exception is not considered breaking.
40704	Bug	40787	`7829d4a`	Non-ASCII dictionary key names and object property names are always escaped.	Reported by the community. Non-ASCII locales have additional JSON payload size and in general this is not expected.	Low. The fix has been in master for several weeks. Appropriate tests added.
	Perf	40889	`27ddbe9`	Minor deserialization perf improvements for collection by removing unnecessary allocations.	(perf)	Low. The fix has been in master for several weeks.
	Perf	40998	`d3c6628`	Major perf deserialzation perf gains primarily due to changes to property name lookup.	(perf)	Low. The fix has been in master for several weeks. Appropriate tests added.
	Perf	41098	`017a038`	Major serialization perf improvements based on [AggressiveInlining] and enumerator changes.	(perf)	Low. The fix has been in master for several weeks.
	Perf	41238	`527595f`	Medium perf gains on deserialize and serialize using a variety of changes.	(perf)	Low. The fix has been in master for two weeks. Appropriate tests added.
	Perf	41363	`4cfc7ec`	Minor perf gains with the JsonCamelCaseNamingPolicy. This decreases warmup time but can help at runtime when using the naming policy on Dictionaries.	(perf)	Low. The fix has been in master for two weeks.
	Perf	41414	`8655ef9`	Minor perf gains on deserialization; includes some clean-up and addresses late comments from previous PR.	(perf)	Low. Appropriate tests added.
41425	Bug	41653	`4580a4d`	A [JsonConverter] attribute when applied to a collection property to specify a custom converter does not work (does nothing).	Reported by the community. Having a broken feature is confusing\not expected when encountered. Note there is a work-around to add the converter at run-time (instead of an attribute).	Low. The fix is contained in a single method and new tests added.

ahsonkhan · 2019-10-14T20:44:54Z

Question: How much effort would it be to port #40867 as well? Is none of the changes from that PR relevant anymore (it looks like that's the case)? I understand that the code changed since then, but it might be good to keep that history of changes in sync.

That's the only PR tagged for perf not included here (6 of 7 targeted for 5.0 are):
https://github.com/dotnet/corefx/pulls?q=is%3Apr+label%3Aarea-System.Text.Json+label%3Atenet-performance+is%3Aclosed+milestone%3A5.0

Also, were the commits here applied in the same order as they were checked-in to master?

ahsonkhan · 2019-10-14T20:53:25Z

...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleDictionary.cs

@@ -159,7 +159,8 @@ public static partial class JsonSerializer
                    }
                }

-                writer.WritePropertyName(key);
+                JsonEncodedText escapedKey = JsonEncodedText.Encode(key, options.Encoder);


This is an incorrect change. It is undoing the optimizations from #40991 (comment) / #41210

Please verify that nothing else from that PR is being changed, unintentionally. Also, how did this diff fall through?

Thanks! Updated that and re-verified other commits.

Also, how did this diff fall through?

It didn't. That fix was done in an earlier change and was out-of-order and manually applied.

steveharter · 2019-10-14T22:24:52Z

How much effort would it be to port #40867 as well?

The code that was changed with #40867 was replaced with #41098 (included here)

steveharter · 2019-10-14T22:49:26Z

Also, were the commits here applied in the same order as they were checked-in to master?

Yes

…es (dotnet#40787)

…tnet#40998)

steveharter · 2019-10-15T21:34:49Z

I did a manual diff between 5.0 and this PR and found this perf PR (#40114) that I just adorned with "performance" label.

It has minor perf benefits, but is the only current 5.0 perf issue AFAIK not ported here. We should consider adding here (or in a subsequent PR).

ericstj · 2019-10-15T21:39:00Z

We can do in subsequent PR. I've reviewed all the commits and am OK with bringing them over to 3.1. We just need to document #40787.

danmoseley · 2019-10-16T01:19:31Z

As far as I/tactics are concerned, this can be merged.

BTW if someone accidentally squashes, I suggest reversing that commit and then doing it again so we have that nice history. I only mention this because I can totally imagine myself doing this 😄

ahsonkhan · 2019-10-16T22:24:56Z

...em.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterKeyValuePair.cs

+        // "encoder: null" is used since the literal values of "Key" and "Value" should not normally be escaped
+        // unless a custom encoder is used that escapes these ASCII characters (rare).
+        // Also by not specifying an encoder allows the values to be cached statically here.
+        private static readonly JsonEncodedText _keyName = JsonEncodedText.Encode(KeyName, encoder: null);


Out of scope of this PR, but we should pass in the user-defined encoder for this (and not null). Though unlikely, if a user creates an encoder that escapes everything, then the payload would be incorrect.

Raise JsonException when PolicyProperty is null (dotnet#40501)

5029c23

steveharter added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Text.Json labels Oct 14, 2019

steveharter added this to the 3.1 milestone Oct 14, 2019

steveharter requested review from danmoseley, ahsonkhan and ericstj October 14, 2019 20:01

steveharter self-assigned this Oct 14, 2019

ahsonkhan reviewed Oct 14, 2019

View reviewed changes

steveharter added 8 commits October 14, 2019 18:21

Use options.Encoder when serializing dictionary keys and property nam…

17c3103

…es (dotnet#40787)

Minor deserialization perf improvements for collections (dotnet#40889)

aaf7fc0

Improve deserialization perf with changes to property name lookup (do…

657f6a5

…tnet#40998)

Serialization performance improvements (dotnet#41098)

7673f84

Serializer perf improvements (dotnet#41238)

9bf90cc

Avoid ToCharArray allocation in JsonCamelCaseNamingPolicy (dotnet#41363)

002ab8a

Address misc feedback and issues from recent perf changes (dotnet#41414)

d1335cc

Support JsonConverterAttribute on enumerable properties (dotnet#41653)

e2da339

steveharter force-pushed the PortJsonIssuesTo31 branch from d9e1ea7 to e2da339 Compare October 14, 2019 23:22

danmoseley added the * NO SQUASH * The PR should not be squashed label Oct 15, 2019

ericstj added the Servicing-consider Issue for next servicing release review label Oct 15, 2019

ahsonkhan reviewed Oct 16, 2019

View reviewed changes

ahsonkhan approved these changes Oct 16, 2019

View reviewed changes

steveharter merged commit 28875ae into dotnet:release/3.1 Oct 16, 2019

steveharter deleted the PortJsonIssuesTo31 branch October 16, 2019 22:28

steveharter removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port perf changes and issues #41771

Port perf changes and issues #41771

steveharter commented Oct 14, 2019

ahsonkhan commented Oct 14, 2019 •

edited

Loading

ahsonkhan Oct 14, 2019 •

edited

Loading

steveharter Oct 14, 2019

steveharter Oct 15, 2019

steveharter commented Oct 14, 2019

steveharter commented Oct 14, 2019

steveharter commented Oct 15, 2019

ericstj commented Oct 15, 2019

danmoseley commented Oct 16, 2019

ahsonkhan Oct 16, 2019

Port perf changes and issues #41771

Port perf changes and issues #41771

Conversation

steveharter commented Oct 14, 2019

ahsonkhan commented Oct 14, 2019 • edited Loading

ahsonkhan Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

steveharter Oct 14, 2019

Choose a reason for hiding this comment

steveharter Oct 15, 2019

Choose a reason for hiding this comment

steveharter commented Oct 14, 2019

steveharter commented Oct 14, 2019

steveharter commented Oct 15, 2019

ericstj commented Oct 15, 2019

danmoseley commented Oct 16, 2019

ahsonkhan Oct 16, 2019

Choose a reason for hiding this comment

ahsonkhan commented Oct 14, 2019 •

edited

Loading

ahsonkhan Oct 14, 2019 •

edited

Loading