Skip to content

Improve HeaderSplit perf #9309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 12, 2019
Merged

Improve HeaderSplit perf #9309

merged 5 commits into from
Apr 12, 2019

Conversation

BrennanConroy
Copy link
Member

Method Mean Error StdDev Op/s Scaled ScaledSD Gen 0 Allocated
SplitSingleHeaderOriginal 344.9 ns 6.914 ns 18.455 ns 2,899,279.9 1.00 0.00 0.0057 192 B
SplitSingleHeader 192.0 ns 4.087 ns 4.542 ns 5,207,375.6 0.56 0.03 0.0038 120 B
Method Mean Error StdDev Op/s Scaled ScaledSD Gen 0 Allocated
SplitSingleQuotedHeaderOriginal 364.5 ns 6.933 ns 6.485 ns 2,743,668.9 1.00 0.00 0.0072 232 B
SplitSingleQuotedHeader 221.6 ns 4.455 ns 6.245 ns 4,513,130.2 0.61 0.02 0.0048 160 B
Method Mean Error StdDev Op/s Scaled ScaledSD Gen 0 Allocated
SplitDoubleHeaderOriginal 441.8 ns 8.725 ns 10.715 ns 2,263,329.0 1.00 0.00 0.0072 200 B
SplitDoubleHeader 259.9 ns 5.057 ns 7.253 ns 3,847,705.3 0.59 0.02 0.0038 128 B
Method Mean Error StdDev Op/s Scaled ScaledSD Gen 0 Allocated
SplitManyHeadersOriginal 852.8 ns 11.214 ns 9.364 ns 1,172,649.9 1.00 0.00 0.0105 320 B
SplitManyHeaders 615.8 ns 11.767 ns 14.451 ns 1,623,843.3 0.72 0.02 0.0076 248 B

Also did an experimental change and got:

Method Mean Error StdDev Median Op/s Scaled ScaledSD Gen 0 Allocated
SplitSingleHeaderOriginal 388.1 ns 16.972 ns 50.043 ns 374.8 ns 2,576,895.9 1.00 0.00 0.0057 192 B
SplitSingleHeader 191.9 ns 3.781 ns 4.045 ns 190.6 ns 5,212,160.9 0.50 0.06 0.0038 120 B
SplitSingleHeaderExperimental 142.4 ns 2.996 ns 6.188 ns 141.8 ns 7,023,061.2 0.37 0.05 0.0010 32 B

But it relied on moving some of the parsing logic into the HeaderSplit method.

@davidfowl
Copy link
Member

This PR:

                  Method |     Mean |     Error |    StdDev |        Op/s |  Gen 0 | Allocated |
------------------------ |---------:|----------:|----------:|------------:|-------:|----------:|
       SplitSingleHeader | 154.3 ns |  3.957 ns | 11.225 ns | 6,480,460.2 | 0.0038 |     120 B |
 SplitSingleQuotedHeader | 173.1 ns |  3.315 ns |  3.101 ns | 5,776,599.5 | 0.0052 |     160 B |
       SplitDoubleHeader | 226.7 ns |  5.713 ns | 16.665 ns | 4,410,972.3 | 0.0043 |     128 B |
        SplitManyHeaders | 615.6 ns | 19.972 ns | 58.574 ns | 1,624,456.3 | 0.0067 |     248 B |

My slight modification:

                  Method |     Mean |     Error |    StdDev |    Median |        Op/s |  Gen 0 | Allocated |
------------------------ |---------:|----------:|----------:|----------:|------------:|-------:|----------:|
       SplitSingleHeader | 101.4 ns |  2.055 ns |  5.695 ns |  99.54 ns | 9,866,116.7 |      - |       0 B |
 SplitSingleQuotedHeader | 146.0 ns |  4.157 ns | 11.993 ns | 144.32 ns | 6,849,742.2 | 0.0014 |      40 B |
       SplitDoubleHeader | 204.0 ns |  4.825 ns | 14.226 ns | 202.31 ns | 4,901,978.0 | 0.0010 |      40 B |
        SplitManyHeaders | 707.5 ns | 14.122 ns | 18.362 ns | 704.63 ns | 1,413,467.7 | 0.0086 |     280 B |

SplitManyHeaders is slower.

@analogrelay analogrelay added this to the 3.0.0-preview5 milestone Apr 12, 2019
foreach (var segment in new HeaderSegmentCollection(values))
{
if (!StringSegment.IsNullOrEmpty(segment.Data))
{
var value = DeQuote(segment.Data.Value);
if (!string.IsNullOrEmpty(value))
{
yield return value;
result = StringValues.Concat(in result, value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't use in now that StringValues is only a pointer size dotnet/extensions#1283

OTOH there isn't currently a Concat overload that takes (StringValues values, string value) without the in so may also want to add that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can look at this in future changes. How much would it help?

@BrennanConroy
Copy link
Member Author

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact ryanbrandenburg.

I've triaged the above build.

@analogrelay
Copy link
Contributor

Related to #3737 ?

@analogrelay
Copy link
Contributor

It looks like this PR will make #3737 irrelevant since you're inlining the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants