[release/3.1] Use Sse2 instrinsics to optimize NeedsEscaping/FindFirstCharToEncode for all built-in JavaScriptEncoders #42030

ahsonkhan · 2019-10-23T11:59:11Z

Port the set of performance optimizations related to escaping checks: #41845, #41933, #42023 and add necessary fixes that are dependent on the release/3.1 branch (such as use of TFM based constants instead of the SDK one)

The PR descriptions highlight the performance improvements. Here's a gist with the analysis:
https://gist.github.com/ahsonkhan/c566f5e7d65c1fde5a83a67be290c4ee

Description

No functional/behavioral change is intended by this change, other than perf optimizations.

Here are the key improvements:

Use Sse2 intrinsics to process 8 characters at a time, otherwise fall back to the sequential code path
Use a int[] cache to store the ASCII characters as they get processed for better caching perf
Loop unrolling in areas where perf matters and it was feasible

This set of optimizations was applied to all the built-in concrete encoders (Default, UnsafeRelaxedJsonEscaping, ones created from the Create factory method, and the TextEncoder virtual methods), and since it was applied on multiple types, some of this change involves refactoring out the common code into helpers/separate files.

Customer Impact:

Callers of the S.T.Json JsonSerializer and Utf8JsonWriter get faster when using any encoder options, particularly for writing strings that are large (>= 16 characters). This change also targets improving users of ASP.NET since they set the JsonSerializerOption to use the non-default JavaScriptEncoder by default (UnsafeRelaxedJsonEscaping).

Here's a summary of the results (regardless of whether folks use the default encoder or pass in the custom built-in encoders via the JsonSerializerOptions or JsonWriterOptions).

For end-to-end scenario (such as serializing commonly found objects/payloads), there is a 10-15% improvement. This is particularly noticeable for an object model/payload used by the NuGet client team (SearchResult).
Writing relatively large JSON strings using the writer got ~30% faster (i.e. greater than 16 characters).
Checking to escape UTF-16 (for strings of length >= 16)
- using Default improved 2x
- using Relaxed improved 3x
- using Custom improved 15-20%
- small regression for lengths <= 4.
Checking to escape UTF-8 (for strings of length >= 16)
- using Default improved 2-5x
- using Relaxed improved 6-10x
- using Custom improved 2-3x
- small regression for lengths <= 2.

By moving the "NeedsEscaping" logic to its correct location - S.T.E.Web and removing duplicate code from S.T.Json leads to better separation of concerns/single responsibility and helps with maintenance by mitigating potential bugs that could occur when we have duplicate logic.

Regression?

No

Risk

The primary risk with this change is the escaping check breaks for some edge case/length. This is mitigated by code review from multiple folks and exhaustive tests (test coverage of the added code is ~100%). The code was also written defensively using Debug.Asserts aggressively to reason about the invariant. Given the performance gains is substantial, and moving the logic into S.T.E.W follows better encapsulation, I think the risk is justified.

Tests run / added

A big part of this change includes adding tests for various encoders and input strings to validate that the right index is being returned for which character needs to be escaped. This includes surrogate pairs, invalid strings, empty strings, etc.

cc @GrabYourPitchforks, @steveharter, @ericstj, @danmosemsft

…N strings (dotnet#41845) * Use Sse2 instrinsics to make NeedsEscaping check faster for large strings. * Update the utf-8 bytes needsescaping and add tests. * Remove unnecessary bitwise OR and add more tests * Add more tests around surrogates, invalid strings, and characters > short.MaxValue.

…xed using Sse2 intrinsics. (dotnet#41933) * Optimize FindFirstCharToEncode for JavaScriptEncoder.Default and Relaxed using Sse2 intrinsics. * Create an Sse2Helper and improve perf of TextEncoder and AllowedCharactersBitmap * Loop unroll FindFirstCharacterToEncode * Improve code coverage. * Add more tests for surrogate pairs and fix call to WillEncode. * Address PR feedback - remove some code duplication. * Move DefaultJavaScriptEncoder to separate file and override EncodeUtf8 with better caching. * Add default replacement character as a test. * Address nits.

…sEscaping (dotnet#42023) * When encoder is null, use JavaScriptEncoder.Default to check for NeedsEscaping. * Remove unnecessary unsafe keyword and add comment to using directive. * Address feedback. * Remove gotos and move the IsEmpty check outside the fixed block.

ahsonkhan · 2019-10-23T12:00:22Z

src/System.Text.Encodings.Web/src/System/Text/Encodings/Web/DefaultJavaScriptEncoder.cs

+
+namespace System.Text.Encodings.Web
+{
+    internal sealed class DefaultJavaScriptEncoder : JavaScriptEncoder


Most of the changes in this file are copy-paste from JavaScriptEncoder.cs into a separate file and don't change the existing logic.

ahsonkhan · 2019-10-23T12:01:51Z

src/System.Text.Encodings.Web/src/System/Text/Encodings/Web/JavaScriptEncoderHelper.cs

+
+namespace System.Text.Encodings.Web
+{
+    internal static class JavaScriptEncoderHelper


This file is extracting out common code into helpers to avoid duplication but is unchanged from what's already there.

ahsonkhan · 2019-10-23T12:04:25Z

...ystem.Text.Encodings.Web/src/System/Text/Encodings/Web/DefaultJavaScriptEncoderBasicLatin.cs

+#if BUILDING_INBOX_LIBRARY
+            if (Sse2.IsSupported)
+            {
+                short* startingAddress = (short*)text;
+                while (textLength - 8 >= idx)
+                {
+                    Debug.Assert(startingAddress >= text && startingAddress <= (text + textLength - 8));
+
+                    // Load the next 8 characters.
+                    Vector128<short> sourceValue = Sse2.LoadVector128(startingAddress);
+
+                    // Check if any of the 8 characters need to be escaped.
+                    Vector128<short> mask = Sse2Helper.CreateEscapingMask_DefaultJavaScriptEncoderBasicLatin(sourceValue);
+
+                    int index = Sse2.MoveMask(mask.AsByte());
+                    // If index == 0, that means none of the 8 characters needed to be escaped.
+                    // TrailingZeroCount is relatively expensive, avoid it if possible.
+                    if (index != 0)
+                    {
+                        // Found at least one character that needs to be escaped, figure out the index of
+                        // the first one found that needed to be escaped within the 8 characters.
+                        Debug.Assert(index > 0 && index <= 65_535);
+                        int tzc = BitOperations.TrailingZeroCount(index);
+                        Debug.Assert(tzc % 2 == 0 && tzc >= 0 && tzc <= 16);
+                        idx += tzc >> 1;
+                        goto Return;
+                    }
+                    idx += 8;
+                    startingAddress += 8;
+                }
+
+                // Process the remaining characters.
+                Debug.Assert(textLength - idx < 8);
+            }
+#endif


This is the crux of the optimization (applied to both utf-16 and utf-8 overloads for the different JavaScriptEncoders).

Tornhoof · 2019-10-23T18:01:30Z

src/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.Escaping.cs

-            // null pointers and gaurd against that. Hence, check up-front and fall down to return -1.
-            if (encoder != null && !value.IsEmpty)
+            // Some implementations of JavaScriptEncoder.FindFirstCharacterToEncode may not accept
+            // null pointers and gaurd against that. Hence, check up-front to return -1.


Tiny Typo: gaurd -> guard.

…M (nc3.0).

steveharter

Based on previous review, the changes look correct. However the S.T.E.W owners should approve this since it changes code in that assembly plus adds a netcoreapp30 config that needs to be vetted.

danmoseley · 2019-10-23T23:27:11Z

Not for Preview 2. Seems too large for 3.1 but let's talk next week. Great change for master.

tarekgh · 2019-10-23T23:46:32Z

src/System.Text.Encodings.Web/src/System/Text/Encodings/Web/DefaultJavaScriptEncoder.cs

+                        Debug.Assert(opStatus == OperationStatus.Done);
+                        idx += utf8BytesConsumedForScalar;
+                    }
+                }


does it make sense to add assert after this line idx == utf8Text.Length?

The while loop condition already guards for idx < utf8Text.Length, so adding the assert would be primarily to make sure idx isn't greater than utf8Text.Length. Sure, we can add it.

Addressed this nit in #42064

ahsonkhan · 2019-10-25T00:12:30Z

Closing this. I'll re-evaluate if we want this change in 3.1.

ahsonkhan and others added 7 commits October 23, 2019 01:19

Add necessary using directive in tests.

28a5bea

Move using directive within ifdef to make it clear when its used.

de7b0f3

Use a custom constant for net core app rather than one used by the SDK.

891e315

Add more tests for custom text encoder case.

7b71e1a

Dotnet-GitSync-Bot added the area-System.Text.Encodings.Web label Oct 23, 2019

ahsonkhan added this to the 3.1 milestone Oct 23, 2019

ahsonkhan commented Oct 23, 2019

View reviewed changes

Tornhoof reviewed Oct 23, 2019

View reviewed changes

ahsonkhan added Servicing-consider Issue for next servicing release review tenet-performance Performance related issue labels Oct 23, 2019

Fix typo in comment gaurd -> guard

7ae6bf1

ahsonkhan mentioned this pull request Oct 23, 2019

Add tests for custom JavaScriptEncoder to cover the virtual code paths in TextEncoder, and address previous feedback. #42064

Merged

danmoseley requested a review from tannergooding October 23, 2019 22:07

Update the S.T.E.W configurations to explicitly target a versioned TF…

7ffd8df

…M (nc3.0).

ahsonkhan requested review from GrabYourPitchforks and steveharter October 23, 2019 22:19

steveharter approved these changes Oct 23, 2019

View reviewed changes

ahsonkhan requested a review from tarekgh October 23, 2019 23:28

tarekgh reviewed Oct 23, 2019

View reviewed changes

tarekgh approved these changes Oct 24, 2019

View reviewed changes

ahsonkhan closed this Oct 25, 2019

ahsonkhan deleted the PortEscapingOptimizations branch October 25, 2019 01:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[release/3.1] Use Sse2 instrinsics to optimize NeedsEscaping/FindFirstCharToEncode for all built-in JavaScriptEncoders #42030

[release/3.1] Use Sse2 instrinsics to optimize NeedsEscaping/FindFirstCharToEncode for all built-in JavaScriptEncoders #42030

Uh oh!

ahsonkhan commented Oct 23, 2019

Uh oh!

ahsonkhan Oct 23, 2019

Uh oh!

ahsonkhan Oct 23, 2019

Uh oh!

ahsonkhan Oct 23, 2019

Uh oh!

Tornhoof Oct 23, 2019

Uh oh!

steveharter left a comment

Uh oh!

danmoseley commented Oct 23, 2019

Uh oh!

tarekgh Oct 23, 2019

Uh oh!

ahsonkhan Oct 24, 2019

Uh oh!

ahsonkhan Oct 25, 2019

Uh oh!

ahsonkhan commented Oct 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[release/3.1] Use Sse2 instrinsics to optimize NeedsEscaping/FindFirstCharToEncode for all built-in JavaScriptEncoders #42030

[release/3.1] Use Sse2 instrinsics to optimize NeedsEscaping/FindFirstCharToEncode for all built-in JavaScriptEncoders #42030

Uh oh!

Conversation

ahsonkhan commented Oct 23, 2019

Description

Customer Impact:

Regression?

Risk

Tests run / added

Uh oh!

ahsonkhan Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

Tornhoof Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

steveharter left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Oct 23, 2019

Uh oh!

tarekgh Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Oct 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants