Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Mar 3, 2020

Fixes #4716

I haven't benchmarked anything yet but response HEADERS frames in unit tests are expectedly smaller.

There are a lot of test failures, but that is because I haven't updated the expected length for HEADERS. Will deal with those once we're happy with the approach.

@JamesNK JamesNK requested a review from a team March 3, 2020 05:38
@JamesNK JamesNK added this to the 5.0.0-preview2 milestone Mar 3, 2020
@benaadams
Copy link
Member

If you are adding an enum KnownHeaderType change it to flags and then use it for the constants?

- if ((_bits & 0x1L) != 0)
+ if ((_bits & KnownHeaderType.CacheControl) != 0)
  {
      _current = new KeyValuePair<string, StringValues>(HeaderNames.CacheControl, _collection._headers._CacheControl);
      _currentKnownType = KnownHeaderType.CacheControl;
      _next = 1;
      return true;
  }

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Any luck cleaning up HPackEncoder.BeginEncode?

Any influence on benchmarks?

I was going to ask about static value encoding, but status is the only response header with values in the static table isn't it?

Not your problem, but still not pleased about the duplication here between the client and the server.

return true;
}

private static bool EncodeHeader(KnownHeaderType knownHeaderType, string name, string value, Span<byte> buffer, out int length)
Copy link
Member

Choose a reason for hiding this comment

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

I had envisioned this method and the enum being in the shared code, but I can understand why you would want to stop here. The client has almost this exact code for request headers but it will take some more work to deduplicate.

Copy link
Member Author

@JamesNK JamesNK Mar 3, 2020

Choose a reason for hiding this comment

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

We're at least now using the same static helper methods.

I'm not sure where the client would use a method like EncodeHeader. Many of their headers are written explicitly, and their unknown headers are written here - https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L1021

I'm not sure how their header buffer handles splitting content across multiple HEADER frames.

Is there somewhere else in the client that I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

I meant that translations like HPackHeaderMapping.GetResponseHeaderStaticTableId would be done inside the HPack implementation.

@JamesNK
Copy link
Member Author

JamesNK commented Mar 3, 2020

If you are adding an enum KnownHeaderType change it to flags and then use it for the constants?

Maybe. KnownHeaderType enum has all the known request and response values in it. It might be too large for a long flags enum.

We might end up using KnownHeaderType as an index in an array of header metadata. That metadata would include the static compression ID, pre-encoded header names, etc. A flag enum wouldn't work in that situation.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

Mainly waiting on HPackEncoder.BeginEncode cleanup and benchmarks.

@JamesNK JamesNK force-pushed the jamesnk/http2-hpackstatic branch from 8fd9655 to c704590 Compare March 3, 2020 23:29

namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2
{
internal static class HPackHeaderWriter
Copy link
Member Author

Choose a reason for hiding this comment

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

This is copy and paste (with some modifications to remove some internal type dependencies) but that appears to match the other code in http2cat which is copied from Http2TestBase


namespace System.Net.Http.HPack
{
internal class HPackEncoder
internal static class HPackEncoder
Copy link
Member Author

Choose a reason for hiding this comment

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

State gone! @scalablecory @Tratcher

@JamesNK JamesNK force-pushed the jamesnk/http2-hpackstatic branch from 294b411 to b1d9b53 Compare March 5, 2020 01:32
@JamesNK JamesNK merged commit 5aa8687 into master Mar 5, 2020
@JamesNK JamesNK deleted the jamesnk/http2-hpackstatic branch March 5, 2020 04:08
@amcasey amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement HPack static compression
4 participants