Remove unsafe from PathNormalizer #56805
Conversation
ladeak
requested review from
halter73,
BrennanConroy,
JamesNK and
mgravell
as code owners
dotnet-issue-labeler
bot
added
the
area-networking
dotnet-policy-service
bot
added
the
community-contribution
|
The build issue seems like an infra issue. |
|
/benchmark plaintext aspnet-citrine-lin kestrel |
|
Benchmark started for plaintext on aspnet-citrine-lin with kestrel. Logs: link |
|
These benchmark results are internal only, right? |
plaintext - aspnet-citrine-lin
|
|
@ladeak what do you mean by "internal only". These are public benchmarks we run continuously (https://github.com/aspnet/benchmarks) but in this case we have a bot that can pull and build an external PR to run them and report the results automatically. |
|
At the time I was trying to access the 'Logs: link', I saw the results posted later. What is the variance of these results? |
|
Numbers are in the standard deviation:
Even when including the value that changed a lot between two runs
|
|
@halter73 @BrennanConroy the bot comments are flowing again (I think they might just be broken for the runtime repository). |
ladeak
force-pushed
the
ladeak-56534-readpointer
branch
from
ac18517 to
4f78231
Compare
|
@amcasey - it seems @BrennanConroy is on holiday. Do you happen to know if these tests fail in general/flaky? (they seem to fail for me on main branch too) |
ladeak
force-pushed
the
ladeak-56534-readpointer
branch
from
0925301 to
8590b1b
Compare
|
They failed in my PR too 😆 |
|
Thank you for confirming. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@adityamandaleeka / @BrennanConroy could someone help to review the changes? Not sure if people are on holiday or on other leave (I hope the former). I would be happy to follow up on the open discussion points. It is also not urgent from my side if it is about waiting a few days or a release candidate - I am happy to wait. Just please keep me posted so I can also align my plans. |
|
We're just busy with other work right now. I do plan on reviewing sometime this week. |
| slashDot.CopyTo(src[writtenLength..]); | ||
| writtenLength += 2; |
There was a problem hiding this comment.
Sure, but that's fine?
In the case below where we have else if (remainingLength == 3) then we can avoid an extra iteration of the loop.
| @@ -2,6 +2,7 @@ | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
There was a problem hiding this comment.
We should probably do a follow up PR to share this file
There was a problem hiding this comment.
I can follow up if I don't forget (holidays coming soon that wipe my memory)
|
Sorry, we meant to fix this test a couple months ago... This test uses the httpsys.dll binary we produce and tests caching with http.sys. But since the default cache limit is ~16MB, if the dll size increases above that it fails the test. Could you add [QuarantinedTest("new issue")] to the test or make it send a small file we create before-hand, up to you (I'd recommend quarantining and we'll fix later)
|
|
I have added the QuarantinedTest attribute. |
|
Thanks @ladeak! |
- Follow up on comment: dotnet#56805 (comment) - Moving PathNormalizer from Shared\HttpSys to Shared\PathNormalizer, changing the namespace to AspNetCore.Internal - Shares the code between HttpSys and Kestrel.Core - Remaining Kestrel specific code renamed to PathDecoder - Keeping the tests and benchmarks in Kestrel
- Follow up on comment: dotnet#56805 (comment) - Moving PathNormalizer from Shared\HttpSys to Shared\PathNormalizer, changing the namespace to AspNetCore.Internal - Shares the code between HttpSys and Kestrel.Core - Remaining Kestrel specific code renamed to PathDecoder - Keeping the tests and benchmarks in Kestrel
* Merging PathNormalizer Implementations - Follow up on comment: #56805 (comment) - Moving PathNormalizer from Shared\HttpSys to Shared\PathNormalizer, changing the namespace to AspNetCore.Internal - Shares the code between HttpSys and Kestrel.Core - Remaining Kestrel specific code renamed to PathDecoder - Keeping the benchmarks in Kestrel
Remove unsafe from PathNormalizer
Updated
ContainsDotSegmentsandRemoveDotSegmentsto be safe in Kestrel's PathNormalizer.Description
Using
Span<T>where possible to remove pointer arithmetic and unsafe code. Added some new unit test cases and extended performance comparison fromDotSegmentRemovalBenchmark.While some cases got faster, some got slower. Looking at the performance results, I think the cases that got faster are probably the more life-like scenarios, while the results that are slower may be the less likely to occur. I leave it up to the maintainers to decide which cases should be in focus when it comes to performance.
The idea proposed in the PR uses two integers to track the read and write positions. In my performance comparison this yielded the best (reasonable) performance.
A different implementation uses
Span<T>to track the read position instead of theint readPointer. While to code may be simpler, it is also about 7-8ns slower in theSingleDotSegmentsandDoubleDotSegmentsbenchmarks.There is another implementation which tries to unroll the
IndexOfand copy into vectorized operations - it is not complete; I did not want to spend too much time on it before it is agreed. This second implementation yieldsSingleDotSegments | 56.36 nsresults, while the rest of the measurements are on-par with the *AFTER cases shown below.Performance
Performance results for the current changes:
BEFORE:
AFTER (Commit: Review feedback copy 3 chars when not matched a dot segment)
Linked to #56534