Skip to content

Reduce the size of HeaderDescriptor #64105

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.

Sign up for GitHub

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

Jump to bottom
Merged
MihaZupan merged 3 commits into from
Feb 1, 2022
Merged

Reduce the size of HeaderDescriptor #64105

MihaZupan merged 3 commits into from
Feb 1, 2022

Conversation

@MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jan 21, 2022 •
edited
Loading

Fixes #62847

NonValidatedAdd_NonValidatedEnumerate:

Toolchain RequestHeaders Mean Error StdDev Median Ratio RatioSD Gen 0 Allocated
main 2 118.64 ns 0.274 ns 1.300 ns 118.28 ns 1.00 0.00 0.0253 256 B
unsafe-pr 2 94.69 ns 0.764 ns 3.680 ns 95.17 ns 0.80 0.03 0.0222 224 B
pr 2 92.44 ns 0.269 ns 1.303 ns 92.51 ns 0.78 0.01 0.0222 224 B
main 4 210.50 ns 0.653 ns 3.068 ns 208.73 ns 1.00 0.00 0.0253 256 B
unsafe-pr 4 157.14 ns 0.234 ns 1.118 ns 156.71 ns 0.75 0.01 0.0222 224 B
pr 4 165.77 ns 0.813 ns 3.949 ns 165.00 ns 0.79 0.03 0.0222 224 B
main 6 356.20 ns 0.488 ns 2.366 ns 354.75 ns 1.00 0.00 0.0467 472 B
unsafe-pr 6 279.82 ns 0.794 ns 3.753 ns 277.51 ns 0.79 0.01 0.0372 376 B
pr 6 294.23 ns 0.400 ns 1.970 ns 293.46 ns 0.83 0.01 0.0372 376 B
main 8 484.50 ns 0.629 ns 3.003 ns 482.65 ns 1.00 0.00 0.0467 472 B
unsafe-pr 8 347.89 ns 1.147 ns 5.490 ns 344.33 ns 0.72 0.01 0.0372 376 B
pr 8 355.82 ns 1.256 ns 6.072 ns 353.78 ns 0.73 0.02 0.0372 376 B
main 12 903.92 ns 1.075 ns 5.127 ns 907.11 ns 1.00 0.00 0.0868 880 B
unsafe-pr 12 732.21 ns 0.769 ns 3.643 ns 731.08 ns 0.81 0.00 0.0648 656 B
pr 12 779.44 ns 2.077 ns 10.180 ns 774.05 ns 0.86 0.02 0.0648 656 B
main 16 1,358.70 ns 0.430 ns 2.032 ns 1,358.55 ns 1.00 0.00 0.0858 880 B
unsafe-pr 16 1,170.03 ns 7.485 ns 39.010 ns 1,164.91 ns 0.86 0.03 0.0648 656 B
pr 16 1,259.05 ns 5.470 ns 28.508 ns 1,241.47 ns 0.93 0.02 0.0648 656 B
main 32 3,889.28 ns 17.269 ns 90.002 ns 3,887.74 ns 1.00 0.00 0.1640 1,672 B
unsafe-pr 32 3,580.34 ns 13.858 ns 72.100 ns 3,570.95 ns 0.92 0.03 0.1183 1,192 B
pr 32 4,046.49 ns 23.530 ns 122.632 ns 4,030.22 ns 1.04 0.03 0.1183 1,192 B

SendAsync (Create and serialize the request, parse the response and enumerate 4 response headers):

Toolchain RequestHeaders Mean Error StdDev Median Ratio RatioSD Gen 0 Allocated
main 4 1.644 μs 0.0062 μs 0.0311 μs 1.630 μs 1.00 0.00 0.0935 944 B
original-pr 4 1.639 μs 0.0057 μs 0.0281 μs 1.630 μs 1.00 0.03 0.0858 880 B
pr 4 1.650 μs 0.0038 μs 0.0191 μs 1.653 μs 1.00 0.02 0.0858 880 B
main 8 2.084 μs 0.0013 μs 0.0064 μs 2.082 μs 1.00 0.00 0.1144 1,160 B
original-pr 8 2.055 μs 0.0030 μs 0.0145 μs 2.055 μs 0.99 0.01 0.0992 1,032 B
pr 8 2.026 μs 0.0068 μs 0.0327 μs 2.028 μs 0.97 0.02 0.0992 1,032 B
main 16 3.428 μs 0.0039 μs 0.0185 μs 3.424 μs 1.00 0.00 0.1526 1,568 B
original-pr 16 3.328 μs 0.0044 μs 0.0209 μs 3.324 μs 0.97 0.00 0.1297 1,312 B
pr 16 3.419 μs 0.0141 μs 0.0675 μs 3.446 μs 1.00 0.02 0.1297 1,312 B

@MihaZupan MihaZupan added this to the 7.0.0 milestone Jan 21, 2022
@MihaZupan MihaZupan requested review from a team and geoffkizer January 21, 2022 17:46
@ghost
Copy link

ghost commented Jan 21, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #62847

NonValidatedAdd_NonValidatedEnumerate:

Toolchain RequestHeaders Mean Error StdDev Median Ratio RatioSD Gen 0 Allocated
main 2 160.2 ns 0.29 ns 1.43 ns 160.0 ns 1.00 0.00 0.0610 256 B
pr 2 124.0 ns 0.25 ns 1.25 ns 124.1 ns 0.77 0.01 0.0534 224 B
main 4 324.5 ns 0.84 ns 4.22 ns 324.3 ns 1.00 0.00 0.0610 256 B
pr 4 251.3 ns 0.62 ns 3.15 ns 252.2 ns 0.77 0.02 0.0534 224 B
main 6 595.2 ns 8.47 ns 42.60 ns 567.3 ns 1.00 0.00 0.1125 472 B
pr 6 488.1 ns 1.17 ns 5.87 ns 490.8 ns 0.82 0.05 0.0896 376 B
main 8 724.4 ns 1.48 ns 7.46 ns 721.8 ns 1.00 0.00 0.1125 472 B
pr 8 614.1 ns 0.73 ns 3.66 ns 613.3 ns 0.85 0.01 0.0896 376 B
main 12 1,297.7 ns 2.04 ns 10.36 ns 1,299.7 ns 1.00 0.00 0.2098 880 B
pr 12 1,173.3 ns 3.95 ns 19.66 ns 1,184.3 ns 0.90 0.02 0.1564 656 B
main 16 1,864.4 ns 1.80 ns 9.16 ns 1,862.6 ns 1.00 0.00 0.2098 880 B
pr 16 1,782.5 ns 11.47 ns 57.91 ns 1,811.9 ns 0.96 0.03 0.1564 656 B
main 32 5,049.9 ns 5.34 ns 27.48 ns 5,045.2 ns 1.00 0.00 0.3967 1,672 B
pr 32 4,895.6 ns 24.95 ns 128.69 ns 4,836.3 ns 0.97 0.03 0.2823 1,192 B

SendAsync (Create and serialize the request, parse the response and enumerate 4 response headers)

Toolchain RequestHeaders Mean Error StdDev Median Ratio Gen 0 Allocated
main 4 2.793 us 0.0035 us 0.0177 us 2.789 us 1.00 0.2251 944 B
pr 4 2.765 us 0.0030 us 0.0147 us 2.767 us 0.99 0.2098 880 B
main 8 3.469 us 0.0020 us 0.0099 us 3.469 us 1.00 0.2747 1,160 B
pr 8 3.431 us 0.0072 us 0.0359 us 3.415 us 0.99 0.2441 1,032 B
main 16 5.137 us 0.0048 us 0.0243 us 5.139 us 1.00 0.3738 1,568 B
pr 16 5.120 us 0.0062 us 0.0314 us 5.113 us 1.00 0.3128 1,312 B
Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http
Milestone: 7.0.0

@ghost ghost assigned MihaZupan Jan 21, 2022
MihaZupan
MihaZupan commented Jan 21, 2022
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/Headers/QPackStaticTable.cs
(new HeaderDescriptor(KnownHeaders.Authorization), ""), // 84
(new HeaderDescriptor(KnownHeaders.ContentSecurityPolicy), "script-src 'none'; object-src 'none'; base-uri 'none'"), // 85
(new HeaderDescriptor("early-data"), "1"), // 86
(new HeaderDescriptor("expect-ct"), ""), // 87
Copy link
Member Author

@MihaZupan MihaZupan Jan 21, 2022

Choose a reason for hiding this comment

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

This is an existing bug in HTTP/3 where x-xss-protection and expect-ct lookups/removes would silently fail if added via OnStaticIndexedHeader.

geoffkizer
geoffkizer reviewed Jan 24, 2022
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderDescriptor.cs
public HttpHeaderParser? Parser => (_descriptor as KnownHeader)?.Parser;
public HttpHeaderType HeaderType => _descriptor is KnownHeader knownHeader ? knownHeader.HeaderType : HttpHeaderType.Custom;

public bool Equals(KnownHeader other) => ReferenceEquals(_descriptor, other);
Copy link
Contributor

@geoffkizer geoffkizer Jan 24, 2022

Choose a reason for hiding this comment

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

It's a little weird to call this method "Equals".

I'd suggest calling it "IsKnownHeader(KnownHeader header)" but we already have a method called that.

Copy link
Member Author

@MihaZupan MihaZupan Jan 24, 2022

Choose a reason for hiding this comment

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

Would you find the == operator more fitting here?

@runfoapp runfoapp bot mentioned this pull request Jan 25, 2022
Open
@MihaZupan
Copy link
Member Author

MihaZupan commented Feb 1, 2022

@dotnet/ncl Please take a look

ManickaP
ManickaP approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM thanks

stephentoub
stephentoub reviewed Feb 1, 2022
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/Headers/QPackStaticTable.cs
(new HeaderDescriptor(KnownHeaders.ContentSecurityPolicy), "script-src 'none'; object-src 'none'; base-uri 'none'"), // 85
(new HeaderDescriptor("early-data"), "1"), // 86
(new HeaderDescriptor("expect-ct"), ""), // 87
(new HeaderDescriptor(KnownHeaders.ExpectCT), ""), // 87
Copy link
Member

@stephentoub stephentoub Feb 1, 2022

Choose a reason for hiding this comment

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

Should we be adding KnownHeaders entries for the other new HeaderDescriptor(string) entries in this table?

Copy link
Member Author

@MihaZupan MihaZupan Feb 1, 2022

Choose a reason for hiding this comment

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

That would be my hope as part of #63750, I updated the post to include QPackStaticTable

@MihaZupan MihaZupan merged commit 4207296 into dotnet:main Feb 1, 2022
@MihaZupan MihaZupan mentioned this pull request Feb 14, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danmoseley danmoseley danmoseley left review comments

@stephentoub stephentoub stephentoub approved these changes

@ManickaP ManickaP ManickaP approved these changes

+1 more reviewer

@geoffkizer geoffkizer geoffkizer left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@MihaZupan MihaZupan

Labels

area-System.Net.Http

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider reducing HeaderDescriptor size by using object field

5 participants

@MihaZupan @stephentoub @danmoseley @ManickaP @geoffkizer