Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces benchmark-driven micro-optimizations across fasthttp hot paths (request conversion, header normalization, cookie parsing, HTTP date parsing, and status line formatting) with the goal of reducing CPU time and allocations.
Changes:
- Optimize status-line formatting by pre-growing buffers and adding a fast 3-digit status-code append path.
- Reduce redundant header-key validation work by introducing
normalizeHeaderKeyValidated(...)and using it in parsing hot paths. - Rework cookie parsing to avoid intermediate copies and add a fast RFC1123-GMT date parsing path used by
ParseHTTPDate(...).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
status.go |
Pre-grows status-line buffer and adds a fast path for 3-digit status codes. |
header.go |
Adds normalizeHeaderKeyValidated and uses it in request/response/trailer header parsing to avoid duplicate validation. |
fasthttpadaptor/request.go |
Adds a fast-path origin-form request URI parse and improves request reuse hygiene (NoBody, RequestURI, TransferEncoding). |
cookie.go |
Switches Set-Cookie parsing to a no-copy scanner and uses ParseHTTPDate fast path for Expires. |
bytesconv.go |
Adds an RFC1123 “GMT” fast parser used by ParseHTTPDate. |
bytesconv_timing_test.go |
Adds benchmarks for AppendHTTPDate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ReneWerner87 The security workflow probably needs to be updated. Since min go for fasthttp is not 1.20.x |
|
It should be using 1.24.x, the version of gosec also needs to be updated. |
|
I'll look at your comments over the next few days. |
|
@erikdubbelboer Can you check again? I'm happy to move things around or remove/adjust them. |
|
I made this 219a75b change to make it more readable. - `parseMonth3` (current packed vs original branch-based):
- current: `40.82 ns/op`
- original: `42.89 ns/op`
- improvement: -4.83% (about -2.07 ns/op), `p=0.000`
- allocations: unchanged (`0 B/op`, `0 allocs/op`)
- `isWeekday3` (current packed vs original branch-based):
- current: `25.45 ns/op`
- original: `26.56 ns/op`
- improvement: -4.18% (about -1.11 ns/op), `p=0.000`
- allocations: unchanged (`0 B/op`, `0 allocs/op`) |
|
@erikdubbelboer Can you check again? Should I adjust anything else? |
…ion and streamline request URI parsing
|
@erikdubbelboer 3231343#diff-a19d0a74cdecfd528924eedab5bb64ecd75edc67d8bdcba74ae00855a6c15149 |
is not related to my code changes , just flaky |
erikdubbelboer
left a comment
erikdubbelboer
left a comment
There was a problem hiding this comment.
Super nice! Only a couple more changes and then this can be merged.
| b := []byte(s) | ||
|
|
||
| // Reference: time.Parse is what ParseHTTPDate falls back to. | ||
| stdTime, stdErr := time.Parse(time.RFC1123, s) |
There was a problem hiding this comment.
This should be http.TimeFormat.
|
|
||
| // The fast path must never accept a string that time.Parse | ||
| // rejects, and when it accepts, the result must be identical. | ||
| fastTime, fastOK := parseRFC1123DateGMT(b) |
There was a problem hiding this comment.
It doesn't really make sense to call parseRFC1123DateGMT(b) if you're going to call ParseHTTPDate(b) right after anyways.
| if t, ok := parseRFC1123DateGMT(date); ok { | ||
| return t, nil | ||
| } | ||
| return time.Parse(time.RFC1123, b2s(date)) |
There was a problem hiding this comment.
Can you change this to http.TimeFormat.
net/http also only accepts GMT as timezone.
| b.ReportAllocs() | ||
| s := string(date) | ||
| for range b.N { | ||
| _, _ = time.Parse(time.RFC1123, s) |
| if err != nil { | ||
| exptime, err = time.Parse("Mon, 02-Jan-2006 15:04:05 MST", v) | ||
| if caseInsensitiveCompare(strCookieExpires, k) { | ||
| exptime, err := ParseHTTPDate(v) |
There was a problem hiding this comment.
Cookies are supposed to be GMT as well, but they aren't always. net/http also supports other timezones here. If ParseHTTPDate is only going to support GMT, we need some fallbacks here. There is also the issue that other timezones should parse based on UTC, not based on the current time of the server (that's why it was using time.ParseInLocation).
I suggest to use this new function:
func parseCookieExpires(src []byte) (time.Time, error) {
if t, ok := parseRFC1123DateGMT(src); ok {
return t, nil
}
s := b2s(src)
// UTC-anchored RFC1123 parsing behavior for non-GMT.
t, err := time.ParseInLocation(time.RFC1123, s, time.UTC)
if err == nil {
return t, nil
}
// Legacy cookie date compatibility used by net/http.
return time.Parse("Mon, 02-Jan-2006 15:04:05 MST", s)
}
Performance Improvements Summary
This PR introduces benchmark-driven performance optimizations in core
fasthttphot paths and keeps only changes with statistically significant wins (benchstat,p < 0.05) and no allocation regressions.1) Header parse hot-path: remove redundant key validation work
File:
header.goChanges
normalizeHeaderKeyValidated(...)for call sites where header-key validity has already been verified.normalizeHeaderKey(...)with the validated variant in:Benchmark
BenchmarkRequestHeaderRead:100.90 ns/op94.56 ns/op-6.28%(p=0.026)B/opandallocs/opremained02) Cookie parsing: major reductions in time and allocations
File:
cookie.goRelated helper reuse:
bytesconv.goChanges
Expires.ParseHTTPDate(...)fast pathMon, 02-Jan-2006 15:04:05 MST) for compatibilityParseBytesto a no-copy parsing flow (nextRaw) for intermediate token processing.trimCookieArgNoCopy), with copy only at final field assignment.Benchmark
BenchmarkCookieParseFull:291.6 ns/op,163 B/op,4 allocs/op85.15 ns/op,0 B/op,0 allocs/op-70.8%(about-206 ns/op),-100% B/op,-100% allocs/op3) Status line formatting: much faster and fewer allocations
File:
status.goChanges
Benchmarks
BenchmarkStatusLine99:46.99 ns/op,120 B/op,4 allocs/op19.49 ns/op,48 B/op,1 allocs/op-58.52%(p=0.000)BenchmarkStatusLine200:26.38 ns/op,56 B/op,3 allocs/op11.29 ns/op,24 B/op,1 allocs/op-57.21%(p=0.000)BenchmarkStatusLine512:47.03 ns/op,120 B/op,4 allocs/op18.91 ns/op,48 B/op,1 allocs/op-59.79%(p=0.000)