Improve Uri.UnescapeDataString codepath #16456

safern · 2017-02-25T01:43:59Z

Work for #16040

This improvement is an initial proposal to avoid char[] allocations done in Uri.UnescapeDataString codepath specifically under UriHelper.UnescapeString. This as well reflects to be faster in a local mini-test I wrote source code here -- and also saw some improvements in inclusive and exclusive time in an asp.net test scenario which you can find here.

The main improvements is that now I'm not seeing any char[] allocations in the GC Heap in any of the scenarios coming from the Uri.UnescapeDataString codepath and a little improvement in execution time.

Here is the mini-test I wrote PerfView traces result:

CPU Stacks

Before My Changes

After My Changes

In this test scenario we can see that there is a bigger difference inclusive and exclusive time spent in Uri.UnescapeDataString

GC Heap

Before My Changes

Callers to Char[]

After My Changes

Callers to Char[]

As we can see in this scenarios Uri.UnescapeDataString is allocating 0 Char[] and it reduced the allocations by 50% from 34,000 to 17,000.

Here are the results for the asp.net test scenario:

CPU Stacks

Before My Changes

After My Changes

As we can see after the changes Uri.UnescapeDataString went down on both exclusive (0.4%) and inclusive (0.1%) time.

GC Heap

Before My Changes

Callers to Char[]

After My Changes

Callers to Char[]

As we can see Char[] allocations went down 0.4% and in this particular scenario is around 7,500 less allocations when using ArrayPool. Also we can see on the callers that are creating this objects that if we use ArrayPool there is 0 Char[] allocations in Uri.UnescapeDataString

Now I'm keeping the old UriHelper.UnescapeString code and its dependencies around because in order to delete it there is a lot of work required because it is used a lot from internal APIs.

/cc @stephentoub @jkotas @danmosemsft

Initial commit

stephentoub · 2017-02-25T12:17:28Z

src/System.Private.Uri/src/System/UriExt.cs

-                        c_DummyChar, c_DummyChar, c_DummyChar, unescapeMode, null, false);
-                    return new string(dest, 0, position);
+                    PooledCharArray pooledArray = new PooledCharArray(stringToUnescape.Length);
+                    UriHelper.UnescapeString(stringToUnescape, 0, stringToUnescape.Length, pooledArray, ref position,


This looks broken. PooledCharArray is a struct, so it'll be copied by value into UnescapeString. If UnescapeString then calls GrowAndCopy, it'll be replacing the array in the copy that was passed in, but not this original. Which means a) the resulting string that gets returned will be incorrect, and b) we'll be releasing the same array back into the ArrayPool twice.

This suggests to me we have a test hole in the tests that exercise this stuff. I suggest before pursuing this further, we ensure those holes are filled, at which point I expect this code will cause failures.

In any event, using a struct like this requires a lot of care, and typically the way it's handled is passing the struct around by ref rather than by value, though even then it requires a lot of scrutiny to ensure it's never being copied inappropriately.

You're correct. I didn't think of the fact that GrowAndCopy would be replacing the copy and not the original. Thanks for the heads up. Will update to do it as by ref.

Opened an issue for the test hole: #16513

stephentoub · 2017-02-28T14:24:00Z

Now I'm keeping the old UriHelper.UnescapeString code and its dependencies around because in order to delete it there is a lot of work required because it is used a lot from internal APIs

This makes it very hard to see in the diff what's changed in your implementation. It also results in a lot of duplication. Is it possible to consolidate them, so that, for example, the char[] based one is implemented on top of the PooledCharArray one as a thin wrapper?

safern · 2017-03-01T00:06:07Z

Updated this to implement the char[] UnescapeString on top of the PooledArray one as a wrapper

danmoseley · 2017-03-01T18:24:46Z

src/System.Private.Uri/src/System/UriExt.cs

@@ -499,29 +499,46 @@ internal unsafe bool InternalIsWellFormedOriginalString()
        public static string UnescapeDataString(string stringToUnescape)
        {
            if ((object)stringToUnescape == null)
+            {


{ [](start = 12, length = 1)

fine either way to add braces but just FYI in the specific case of a single line block that is not nested in another block we allow omitting braces

https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

my slight preference is to omit them here as it's easier to read but either is fine

In reply to: 103753759 [](ancestors = 103753759)

Yes I was aware of single line blocks with no braces was allowed in our guidelines, I just added them because in other PRs I've done I have had the experience where the owners asked for braces on those blocks :( so I didn't know which one was the preferred style, but will get rid of them in this case :)

-- Thanks for the heads up by the way.

danmoseley · 2017-03-01T18:26:31Z

src/System.Private.Uri/src/System/UriExt.cs

-                    return new string(dest, 0, position);
+                    PooledCharArray pooledArray = new PooledCharArray(stringToUnescape.Length);
+                    UriHelper.UnescapeString(stringToUnescape, 0, stringToUnescape.Length, ref pooledArray, ref position,
+                    c_DummyChar, c_DummyChar, c_DummyChar, unescapeMode, null, false);


you could add overloads that don't take reserved chars so you don't have to pass in horrible dummy chars. But it was like that already so up to you

danmoseley · 2017-03-01T18:26:44Z

src/System.Private.Uri/src/System/UriExt.cs

+                    UriHelper.UnescapeString(stringToUnescape, 0, stringToUnescape.Length, ref pooledArray, ref position,
+                    c_DummyChar, c_DummyChar, c_DummyChar, unescapeMode, null, false);
+
+                    if(pooledArray.IsSameString(pStr, 0, position))


( [](start = 22, length = 1)

nit:spacing

danmoseley · 2017-03-01T18:45:44Z

src/System.Private.Uri/src/System/PooledCharArray.cs

+    		}
+    	}
+
+        public unsafe bool IsSameString(char* ptrStr, int start, int end)


IsSameString [](start = 27, length = 12)

why not make this private, and instead expose something like

public string GetStringAndRelease(string candidate)

it can get the length off the string you pass in, so no need for a pointer/start/end/length.

also, you can eliminate your Release() method.

it's better to force them to release at the same time they "get what they want" as that way it's hard to forget to Release()

In reply to: 103758643 [](ancestors = 103758643)

danmoseley · 2017-03-01T18:59:01Z

src/System.Private.Uri/src/System/PooledCharArray.cs

+            return true;
+        }
+
+    	public void GrowAndCopy(int extraSpace)


GrowAndCopy [](start = 17, length = 11)

We're trusting callers here to not hold on to the backing buffer through this call.
Might be better to call it something like "ReplaceArray" ? as a hint

What about GrowAndReplaceArray? Is it too long? Just to let them know that it will create a new array the length of the old one + the extraSpace they need.

sure.

In reply to: 103766045 [](ancestors = 103766045)

danmoseley · 2017-03-01T19:54:27Z

src/System.Private.Uri/src/System/UriExt.cs

-                    dest = UriHelper.UnescapeString(stringToUnescape, 0, stringToUnescape.Length, dest, ref position,
-                        c_DummyChar, c_DummyChar, c_DummyChar, unescapeMode, null, false);
-                    return new string(dest, 0, position);
+                    PooledCharArray pooledArray = new PooledCharArray(stringToUnescape.Length);


stringToUnescape.Length [](start = 70, length = 23)

don't we know almost for certain this is too small? since you found at least one %. Should it be a little bigger (2? 5? 10?)

No actually it is in most of the cases getting smaller because when we are unescaping strings we are transforming from escaped characters to regular characters and escaped characters are usually a 2 char or 3 char length string. i.e %20 -> ' '. The case where it has to grow is when the escaped string has reserved characters which is the rare case.

danmoseley · 2017-03-01T19:55:23Z

src/System.Private.Uri/src/System/UriExt.cs

-                    return new string(dest, 0, position);
+                    PooledCharArray pooledArray = new PooledCharArray(stringToUnescape.Length);
+                    UriHelper.UnescapeString(stringToUnescape, 0, stringToUnescape.Length, ref pooledArray, ref position,
+                    c_DummyChar, c_DummyChar, c_DummyChar, unescapeMode, null, false);


false); [](start = 78, length = 8)

it doesn't seem consistent in corefx code but in my previous teams we would always annotate bools in the caller since they're so opaque eg

null, /* isQuery= */ false)

up to you

sometimes we did it for null also

or you can use named arguments instead of comments, isQuery: false

that's right -- seems to be the pattern around here. Iff the parameters are at the end of the signature.

In reply to: 103774696 [](ancestors = 103774696)

danmoseley · 2017-03-01T19:55:59Z

src/System.Private.Uri/src/System/UriExt.cs

+                    {
+                        pooledArray.Release();
+                        return stringToUnescape;
+                    }


as noted in PooledCharArray, I think we can cut this block and just make GetSTringAndRelease do al the work.

danmoseley · 2017-03-01T20:02:13Z

src/System.Private.Uri/src/System/UriHelper.cs

+        internal static unsafe char[] UnescapeString(char* pStr, int start, int end, char[] dest, ref int destPosition,
+            char rsvd1, char rsvd2, char rsvd3, UnescapeMode unescapeMode, UriParser syntax, bool isQuery)
+        {
+            PooledCharArray pooledArray = new PooledCharArray(dest);


new PooledCharArray(dest); [](start = 41, length = 27)

this is a new allocation. but you don't need one, you have dest to write to. are you only doing this because UnescapeString expects a pooled array? If so we should maybe think about how to avoid that.

Well Stephen said we would have duplicate code so it would be better to make this method a wrapper around the UnescapeString that needs the pooled array. Anyway PooledCharArray is a struct so it will not be allocated in the heap, and therefore we are avoiding char[] in this codepath as we are using the PooledCharArray and just copying it over to the dest char[] as a lot of APIs that fall in this codepath expect that behavior.

danmoseley · 2017-03-01T20:02:50Z

src/System.Private.Uri/src/System/UriHelper.cs

+
+            if (destPosition > dest.Length)
+            {
+                dest = new char[destPosition];


new char[destPosition]; [](start = 23, length = 23)

why isn't this getting it from the pool? any use of "new char[]" should be suspect in these codepaths now.

Actually I debugged the tests we currently have and there wasn't a case where it hitted this specific line of code. Eventhough this is just to make sure when copying it over to dest the new string fits in there, but still it wouldn't affect the passed dest because it is copied by value, but therefore we would return the new expanded array. What would you suggest?

danmoseley · 2017-03-01T20:03:50Z

src/System.Private.Uri/src/System/PooledCharArray.cs

+    		return ret;
+    	}
+
+		public void CopyAndRelease(char[] dest, int offset, int count)


offset [](start = 46, length = 6)

you always pass offset=zero so it might just make sense to have a single parameter (length)

danmoseley · 2017-03-01T20:07:46Z

    private void MatchUTF8SequenceOverrunTest(byte[] inbytes, int numBytes, bool isQuery, bool iriParsing)

you verified your test changes here cause a failure with your original iteration?

Refers to: src/System.Private.Uri/tests/UnitTests/IriEscapeUnescapeTest.cs:327 in 6fa9a62. [](commit_id = 6fa9a62, deletion_comment = False)

danmoseley · 2017-03-01T20:10:03Z

src/System.Private.Uri/src/System/PooledCharArray.cs

+            return true;
+        }
+
+    	public void GrowAndCopy(int extraSpace)


GrowAndCopy [](start = 17, length = 11)

Another approach might be to have a new constructor for PooledCharArray that took a PooledCharArray and extraSpace (or "size" to be consistent with the other ctor)

It would do all the same work.

danmoseley · 2017-03-01T20:12:57Z

    internal static unsafe char[] EscapeString(string input, int start, int end, char[] dest, ref int destPos,

you didn't wave your magic wand over this one, and it does allocate char[]. Is that becuase it's not on the hot path? (which is a fair reason)

Refers to: src/System.Private.Uri/src/System/UriHelper.cs:128 in 6fa9a62. [](commit_id = 6fa9a62, deletion_comment = False)

danmoseley · 2017-03-01T20:15:10Z

src/System.Private.Uri/src/System/UriHelper.cs

+
+            if (destPosition > dest.Length)
+            {
+                dest = new char[destPosition];


est = new char[destPosition]; [](start = 17, length = 29)

another allocation.

danmoseley · 2017-03-01T20:23:18Z

namespace System.Net.Test.Uri.IriTest

Block coverage for UriHelper, IriHelper, is ~90% and Uri is 75% which isn't bad but branch coverage is more like 50%

Ideally you'd look at the branch coverage of the code around your changes by getting local code covearge data, then look for the little branch icons in the report showing where branches are missed

https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/

That would probably be expensive though.

In general the size/complexity/exposure of this change is a little worrying.

Refers to: src/System.Private.Uri/tests/UnitTests/IriEscapeUnescapeTest.cs:10 in 6fa9a62. [](commit_id = 6fa9a62, deletion_comment = False)

danmoseley · 2017-03-01T20:25:48Z

namespace System.Net.Test.Uri.IriTest

It might need scoping down a little. Interested in Stephen's tak.e

In reply to: 283458589 [](ancestors = 283458589)

Refers to: src/System.Private.Uri/tests/UnitTests/IriEscapeUnescapeTest.cs:10 in 6fa9a62. [](commit_id = 6fa9a62, deletion_comment = False)

danmoseley · 2017-03-01T20:27:00Z

namespace System.Net.Test.Uri.IriTest

As a general rule if you improved test coverage, you want to do it WITHOUT your implementation changes. Make sure they all pass. Then put your implementation in. That helps detect regressions

In reply to: 283458589 [](ancestors = 283458589)

Refers to: src/System.Private.Uri/tests/UnitTests/IriEscapeUnescapeTest.cs:10 in 6fa9a62. [](commit_id = 6fa9a62, deletion_comment = False)

danmoseley · 2017-03-02T22:23:55Z

@safern @stephentoub I suggest abandoning this PR as it's a fair way from here to mergeable (given complexity) and 2.0/UWP work needs to happen.

We can keep the issue open and reuse the learnings from this PR if and when we take another look, or someone else does. Thanks for the work on it @safern .

safern · 2017-03-02T22:26:22Z

Sounds great! Thanks for all the advises and feedback from this PR it was a very good learning experience! @stephentoub @danmosemsft

safern added 2 commits February 24, 2017 15:30

Initial changes

51eac08

Code cleanup

82b62ca

Initial commit

safern added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net tenet-performance Performance related issue labels Feb 25, 2017

safern added this to the Future milestone Feb 25, 2017

safern self-assigned this Feb 25, 2017

dnfclas added the cla-already-signed label Feb 25, 2017

stephentoub reviewed Feb 25, 2017

View reviewed changes

PooledCharArray by ref parameter

d8bf63b

Address PR Feedback

6fa9a62

danmoseley reviewed Mar 1, 2017

View reviewed changes

danmoseley closed this Mar 2, 2017

karelz modified the milestones: 2.0.0, Future Mar 7, 2017

safern deleted the UnescapeDataString branch April 2, 2018 18:14

safern mentioned this pull request Jan 31, 2020

[Uri] Test hole in System.Private.Uri -> UriHelper.EscapeString dotnet/runtime#20355

Closed

stephentoub mentioned this pull request Jan 31, 2020

[Uri] Uri.ReCreateParts allocates a char[] that could be avoided dotnet/runtime#22903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Uri.UnescapeDataString codepath #16456

Improve Uri.UnescapeDataString codepath #16456

safern commented Feb 25, 2017

stephentoub Feb 25, 2017

safern Feb 27, 2017 •

edited

Loading

stephentoub commented Feb 28, 2017

safern commented Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

safern Mar 1, 2017 •

edited

Loading

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

safern Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

safern Mar 1, 2017

danmoseley Mar 1, 2017

Priya91 Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley Mar 1, 2017

safern Mar 1, 2017 •

edited

Loading

danmoseley Mar 1, 2017

safern Mar 1, 2017

danmoseley Mar 1, 2017 •

edited

Loading

danmoseley commented Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley commented Mar 1, 2017

danmoseley Mar 1, 2017

danmoseley commented Mar 1, 2017

danmoseley commented Mar 1, 2017

danmoseley commented Mar 1, 2017

danmoseley commented Mar 2, 2017

safern commented Mar 2, 2017

Improve Uri.UnescapeDataString codepath #16456

Improve Uri.UnescapeDataString codepath #16456

Conversation

safern commented Feb 25, 2017

Here is the mini-test I wrote PerfView traces result:

CPU Stacks

GC Heap

Here are the results for the asp.net test scenario:

CPU Stacks

GC Heap

Choose a reason for hiding this comment

safern Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub commented Feb 28, 2017

safern commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

danmoseley commented Mar 1, 2017

Choose a reason for hiding this comment

danmoseley commented Mar 1, 2017

Choose a reason for hiding this comment

danmoseley commented Mar 1, 2017

danmoseley commented Mar 1, 2017

danmoseley commented Mar 1, 2017

danmoseley commented Mar 2, 2017

safern commented Mar 2, 2017

safern Feb 27, 2017 •

edited

Loading

safern Mar 1, 2017 •

edited

Loading

safern Mar 1, 2017 •

edited

Loading

danmoseley Mar 1, 2017 •

edited

Loading