Optimize memory allocations in ParseAccept #3
Conversation
wojtek-t
commented
Oct 8, 2019
wojtek-t
commented
|
@munnerz - please take a look @smarterclayton - FYI |
munnerz
left a comment
munnerz
left a comment
There was a problem hiding this comment.
This seems good to me, although I'm not too well-versed in this code. Tracing through, the changes look to be functionally the same (and the unit tests pass!).
I'm not sure of the original author's github details, but the repository this was forked from is here: https://bitbucket.org/ww/goautoneg
autoneg.go
Outdated
| // Parse an Accept Header string returning a sorted list | ||
| // of clauses | ||
| func ParseAccept(header string) (accept []Accept) { | ||
| func ParseAccept(header string) (accept AcceptSlice) { |
There was a problem hiding this comment.
Does it make sense to expose AcceptSlice to consumers of this package given that ParseAccept already sorts the slice anyway? Can this be kept as []Accept to avoid leaking the implementation details to the consumer?
wojtek-t
force-pushed
the
improve_performance_parse_header
branch
from
c1a3fe0 to
c4ea822
Compare
munnerz
left a comment
munnerz
left a comment
There was a problem hiding this comment.
@smarterclayton if you think this looks good, I'll hit merge 😄
autoneg.go
Outdated
| a.Q = 1.0 | ||
|
|
||
| mrp := strings.Split(part, ";") | ||
| accept := make(acceptSlice, 0, len(parts)) |
There was a problem hiding this comment.
You can add another fast path here by not splitting (which allocates) if there's no comma.
There was a problem hiding this comment.
It would actually be better to make this a loop that tokenizes parts on , if you want to avoid allocations. I did that in go-restful to bypass https://github.com/emicklei/go-restful/blob/95fec025ab35f1d26407d1a4ef1542a3bf4eb8a0/route.go#L79
There was a problem hiding this comment.
I was optimizing the k8s usage TBH. And for all calls from k8s client (vast majority) there is a comma actually:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/request.go#L140
It would actually be better to make this a loop that tokenizes parts on , if you want to avoid allocations. I did that in go-restful to bypass https://github.com/emicklei/go-restful/blob/95fec025ab35f1d26407d1a4ef1542a3bf4eb8a0/route.go#L79
I did part of it for part, but it probably makes sense to go further. Will change that.
wojtek-t
force-pushed
the
improve_performance_parse_header
branch
from
c4ea822 to
873bfd1
Compare
wojtek-t
force-pushed
the
improve_performance_parse_header
branch
from
873bfd1 to
da14def
Compare
|
@smarterclayton @munnerz - PTAL |
|
Also benchmark results are better now (only 2 allocations vs 5 and vs 18 without any changes) |
|
lgtm |
3 participants
