x/net/http/httpguts: validation of header names, methods, and cookie names could be about twice as fast

[jub0bs]

Go version

go1.22.2

golang/x/net v0.24.0

Output of go env in your module/workspace:

GOARCH='amd64'
GOOS='darwin'

What did you do?

I noticed that httpguts.IsTokenRune, which httpguts.ValidHeaderFieldName calls iteratively and which net/http in general relies on, systematically incurs a bounds check. I then wrote an alternative implementation that eliminates those bounds checks and also obviates the need for UTF-8 decoding.

package p

import "unicode/utf8"

func ValidHeaderFieldName(v string) bool {
	return isToken(v)
}

func isToken(s string) bool {
	if len(s) == 0 {
		return false
	}
	for i := 0; i < len(s); i++ {
		if !validTokenByte[s[i]] {
			return false
		}
	}
	return true
}

func IsTokenRune(r rune) bool {
	return r < utf8.SelfRune && validTokenByte[byte(r)]
}

var validTokenByte = [256]bool{
	'!':  true,
	'#':  true,
	'$':  true,
	'%':  true,
	'&':  true,
	'\'': true,
	'*':  true,
	'+':  true,
	'-':  true,
	'.':  true,
	'0':  true,
	'1':  true,
	// rest omitted for brevity
	'~':  true,
}

What did you see happen?

Benchmarks indicate that my implementation is about twice as fast as the current one; see below.

Moreover, the same logic could be used to validate HTTP methods and cookie names in addition to HTTP header-field names, since all three share the same production (token). At the moment, their validation logic, because it relies on strings.IndexFunc and httpguts.IsTokenRune, isn't as fast as it could, as shown by one of my benchmarks.

/cc @bradfitz @neild

goos: darwin
goarch: amd64
pkg: github.com/jub0bs/httpguts-perf-exp
cpu: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
                       │     std      │               jub0bs                │
                       │    sec/op    │   sec/op     vs base                │
IsCookieNameValid-8      1615.0n ± 0%   363.8n ± 1%  -77.48% (p=0.000 n=20)
ValidHeaderFieldName-8    669.3n ± 1%   328.1n ± 0%  -50.99% (p=0.000 n=20)
IsTokenRune-8             594.8n ± 0%   592.2n ± 0%   -0.45% (p=0.004 n=20)
geomean                   863.1n        413.4n       -52.10%

                       │     std      │               jub0bs                │
                       │     B/op     │    B/op     vs base                 │
IsCookieNameValid-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20) ¹
ValidHeaderFieldName-8   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20) ¹
IsTokenRune-8            0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20) ¹
geomean                             ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                       │     std      │               jub0bs                │
                       │  allocs/op   │ allocs/op   vs base                 │
IsCookieNameValid-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20) ¹
ValidHeaderFieldName-8   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20) ¹
IsTokenRune-8            0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=20) ¹
geomean                             ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

What did you expect to see?

I expected at least some speedup of httpguts.ValidHeaderFieldName.

I have not yet checked how such a change would affect the performance of net/http, though.

[jub0bs]

@dmitshur This would be my first contribution to Go; I'm still learning the ropes. My understanding of the NeedsInvestigation tag and of the contribution guide is that I should wait for other people to chime in here before sending a PR. Correct?

[seankhliao]

cc @neild

[dmitshur]

@jub0bs Yes, your understanding is right and generally it's good to allow some time for discussion on the issue first, though it's still okay to send a change sooner. There are some more details about the intended use of labels at https://go.dev/wiki/HandlingIssues#issue-states. Thanks.

[neild]

I'm happy to review a CL if you want to send me one.

For purely changes that have no behavioral impact and don't add new API surface, it's generally fine to just send a CL without opening an issue. (Sounds like you might be proposing a new function in httpguts, though? Send me a CL and we can figure it out from there.)

[gopherbot]

Change https://go.dev/cl/578075 mentions this issue: http/httpguts: speed up ValidHeaderFieldName

[jub0bs]

@neild Thanks!

I did float the idea of augmenting httpguts with an IsToken function, which would subsume the logic in ValidHeaderFieldName as well as the logic used for validating method names and cookie names in net/http. However, my understanding of the contribution guide is that such a change would have to go through a proposal. Correct?

Perhaps we should first see if my proposed speedup of ValidHeaderFieldName gets accepted; if it does, I can follow up with such a proposal.