Uri.UnescapeDataString and Uri.UnescapeString reuse the original string when possible#41684
Conversation
|
Can we also see some benchmarks with the before/after with this PR change? See PR #41640 as an example of how we write little benchmarks to help validate a PR and perf gains. |
|
The current perf results. It became ~10% slower after the moving logic out of ValueStringBuilder.
|
What exactly are you comparing? The previous commit you published was buggy, e.g. it wasn't clearing memory it needed to be clearing. If you're now clearing that memory and you weren't before, you're comparing apples to oranges. |
|
The last perf measurement didn't include pooled array cleaning. I just wanted to see if extracting methods out VSB brought in some perf change.
|
Ok, thanks for following up on it. I will take another look on Monday. |
src/System.Private.Uri/tests/UnitTests/IriEscapeUnescapeTest.cs
Outdated
Show resolved
Hide resolved
- Explicit pooled array cleaning is removed
|
After I replaced indexer with Append method and remove the explicit pooled array cleaning as unnecessary (Append guarantees that data is always written before it can be read), the performance improved and got slightly better than in my master version. (Note. My master doesn't include the latest @stephentoub changes, I will merge with them in the next commit)
|
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
- Code style fixes
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
This reverts commit e5919e3.
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
* UriExt.UnescapeDataString avoids a new string allocation for an unmodified unescaped string * Original dest array reuse fixed * ValueStringBuilder is used instead of PooledCharArray * Remove unused type * Changes to ValueStringBuilder reverted * IsStringSame replaced with SequenceEqual * Array returned from buffer is cleared * - Indexer-based assignment is replaced with Append method - Explicit pooled array cleaning is removed * .sln file changes reverted * Build fixed * UnescapeString always copies the result to dest array * Unused code removed * Original string reuse fixed * Heap validation fixed * - Small optimizations - Code style fixes * By-char append is replaced with span-based one * SequenceEqual is invoked against the original string * Revert "By-char append is replaced with span-based one" This reverts commit dotnet/corefx@e5919e3. Commit migrated from dotnet/corefx@dacc255
Uri.UnespaceDataString and Uri.UnescapeString retrieve an array from the pool to use as a temporary data storage for unescaping algorithm instead of a new string or array allocation on each call. After the processing completes, they compare the result stored in the buffer against the original string and return the original object without extra allocation if no changes have been made.
Fixes #16040
Benchmark code
This PR is based on the original idea of @safern