Ability to set http headers without allocations #2119
Comments
|
To start the discussion on this I made a commit with benchmarks and a naive implementation of Internally in yarpc the responsewriters might have a header they build up, so I decided to only benchmark the add-step and not the entire response. Benchmark as follows: This shows that adding a |
|
Hey, Thanks for looking into this! There seems to be a lot to unpack here:
So this would show a clear performance win. However, after a brief look at the code, I'm not sure if the benchmarks are valid - I think we just keep getting errors since we're adding duplicate headers (?). Ultimately though - it should be possible to add headers with 0 allocations, right? |
|
I looked at this a bit more, and tried to reproduce the "header creation doesn't allocate" situation. And it appears:
The code below is somewhat convoluted - tried to reproduce what func newStore1(capacity int) *store1 {
s := store1{
store1: make(map[string]string, 1),
}
return &s
}
func (s *store1) with(k, v string) *store1 {
s.store1[k] = v
return s
}
func do1(out map[string]string, k, v string) {
s := newStore1(1).with(k, v)
out[k] = s.store1[k]
}
type store2 struct {
store1 map[string]string
store2 map[string]string
}
func newStore2(capacity int) *store2 {
s := store2{
store1: make(map[string]string, 1),
store2: make(map[string]string, 1),
}
return &s
}
func (s *store2) with(k, v string) *store2 {
s.store1[k] = v
s.store2[k] = v
return s
}
func do2(out map[string]string, k, v string) {
s := newStore2(1).with(k, v)
out[k] = s.store1[k]
}
func BenchmarkBaseline(b *testing.B) {
out := make(map[string]string, 1)
for i := 0; i < b.N; i++ {
out["me"] = "hello"
}
assert.NotEqual(b, "", out["me"])
}
func BenchmarkStatic1(b *testing.B) {
out := make(map[string]string, 1)
for i := 0; i < b.N; i++ {
do1(out, "me", "hello")
}
assert.NotEqual(b, "", out["me"])
}
func BenchmarkStatic2(b *testing.B) {
out := make(map[string]string, 1)
for i := 0; i < b.N; i++ {
do2(out, "me", "hello")
}
assert.NotEqual(b, "", out["me"])
}Results in: So significantly slower, and the cost grows for each internal map, as expected. Adding So, in summary:
|
|
I found one allocation to save in These are the optimizations: And for the addHeader: So as you say, |
|
I've created a PR with the current optimizations. |
|
@biosvs I don't think this one was actually completed - should we re-open? I still think this would be a significant improvement overall. |
|
Yes, let's re-open this, as performance question was raised recently. I'll take a look into this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hello,
It seems not possible to add headers without allocations. Adding a header seems to be costing 6 (or 4) allocations.
Ideas
My initial thought was that the AddHeaders API is inefficient, but @jstroem below benchmarked this not be a problem.
#### First thought - No "AddHeader" API.however,
transport.Headersalways allocated two mapsyarpc canonical headers are lowercase, while http canonical headers are capitalized.
This leads to a very paradoxical situation where:
if I add lower-case http header
I get:
This means two allocations for the maps in
transport.Headers, 1 allocation foraddToMetadata, and possibly 1 https://pkg.go.dev/net/http#CanonicalHeaderKey - not sure if the last one is included in my benchmark though.But if I do correct HTTP canonical headers, which should be better
I get
so it's worse - yarpc will convert my correct header to lower-case first (extra alloc) and one another extra alloc that I don't understand.
Possible solutions
There are a few options - which one would you recommend?
#### Just add `AddHeader`
It would be relatively easy to add
ResponseWriter.AddHeader(key, value string)- this would save me two allocations, leaving the remaining two.AddRawHeader, or makeAddSystemHeaderpublicIn order to avoid Go triggering https://pkg.go.dev/net/http#CanonicalHeaderKey for me later on anyway, I would rather pass in HTTP canonical headers (
Abcand notabc).This means I would need something like
ResponseWriter.AddRawHeader(key, value string)that wouldn't dotransport.CanonicalizeHeaderKeyfor me. But I'm not sure about the other implications of doing this.Anything else?
Please advise.
The text was updated successfully, but these errors were encountered: