Optimise sanitize with string builder #2918
Conversation
| @@ -51,7 +51,8 @@ func validHeaderFieldByte(b byte) bool { | |||
| } | |||
|
|
|||
| // make sure we don't generate invalid headers | |||
| func sanitize(input string) string { | |||
| // temporary public function to benchmark it | |||
There was a problem hiding this comment.
There is no need to export it as benchmarks sits in the same package.
Usually we introduce benchmark as a first commit and include baseline benchmark results in the commit message.
Then change implementation, re-run benchmark and compare results to the baseline via https://pkg.go.dev/golang.org/x/perf/cmd/benchstat
To get a statistically meaningful benchmark results it should be run several times, usually we run 10.
See this PR as an example of performance-related change #2870
I've also created a small script to automate benchmarking two commits (its important that benchmark function is added in a separate commit to establish the baseline), see golang/go#63233 (comment)
| toAscii := strconv.QuoteToASCII(input) | ||
| var s strings.Builder | ||
| for _, i := range toAscii { | ||
| if validHeaderFieldByte(byte(i)) { | ||
| s.WriteRune(i) | ||
| } | ||
| } | ||
| return s.String() |
There was a problem hiding this comment.
To get ultimate performance we can use byte version https://pkg.go.dev/strconv#AppendQuoteToASCII here and then replace invalid bytes with - instead of allocating another slice.
| } | ||
|
|
||
| for i, v := range table { | ||
| url, _ := url.ParseRequestURI(v.url) |
There was a problem hiding this comment.
This is redundant, test cases should contain query string instead of url
| for i, v := range table { | ||
| url, _ := url.ParseRequestURI(v.url) | ||
| q := url.Query() | ||
| b.Run(fmt.Sprintf("[new sanitize] url %d", i + 1), func(b *testing.B) { | ||
| for i := 0; i < b.N; i++ { | ||
| for k := range q { | ||
| NewSanitize(k) | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
There is no need to have a second benchmark, see comment above.
| @@ -0,0 +1,14 @@ | |||
| `go test -benchmem -run=^$ -bench ^BenchmarkStripQuery$ -benchtime=10s github.com/zalando/skipper/filters/builtin` | |||
There was a problem hiding this comment.
Please add results to the commit message as described in the comment above.
| for i, v := range table { | ||
| url, _ := url.ParseRequestURI(v.url) | ||
| q := url.Query() | ||
| b.Run(fmt.Sprintf("[old sanitize] url %d", i+1), func(b *testing.B) { |
There was a problem hiding this comment.
Makes sense to add b.ReportAllocs() instead of -benchmem flag
not a big difference, but on large query strings it's noticeable.
can be minor and safe enhancement?
go test -benchmem -run=^$ -bench ^BenchmarkStripQuery$ -benchtime=10s github.com/zalando/skipper/filters/builtin