Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filters/builtin: minor performance optimization for sanitize in StripQuery Filter #2940

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

trkohler
Copy link
Member

@trkohler trkohler commented Feb 16, 2024

This is continuation of https://github.com/zalando/skipper/pull/2918/files

I have discovered that for certain cases string builder performs better than buffer, which is usually a go-to option for Go Developers. String builder was introduced in 1.10+ Go.

results (also attached to last commit message)

pkg: github.com/zalando/skipper/filters/builtin
           │   HEAD~1    │                HEAD                │
           │   sec/op    │   sec/op     vs base               │
Sanitize-8   802.1n ± 0%   748.3n ± 0%  -6.71% (p=0.000 n=10)

           │   HEAD~1   │                HEAD                │
           │    B/op    │    B/op     vs base                │
Sanitize-8   472.0 ± 0%   192.0 ± 0%  -59.32% (p=0.000 n=10)

           │   HEAD~1   │               HEAD                │
           │ allocs/op  │ allocs/op   vs base               │
Sanitize-8   15.00 ± 0%   14.00 ± 0%  -6.67% (p=0.000 n=10)

@trkohler trkohler added the minor no risk changes, for example new filters label Feb 16, 2024
@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Feb 16, 2024

Please include benchstat results (without 10+10 before/after benchmark outputs) into last commit message (and update PR description).

I'd also suggest to prefix commit messages with a package name, e.g.:
filters/builtin: introduce benchmark for stripQuery filter

@trkohler trkohler force-pushed the setup-proper-benchmark branch from 90996ab to 6769d14 Compare February 16, 2024 12:29
@trkohler trkohler force-pushed the setup-proper-benchmark branch from 6769d14 to aa75b28 Compare February 16, 2024 12:55
@trkohler trkohler changed the title [wip] improve speed in sanitize through string.builder Improve speed in sanitize through string.builder Feb 16, 2024
@trkohler trkohler force-pushed the setup-proper-benchmark branch from aa75b28 to afdd426 Compare February 16, 2024 18:04
@trkohler trkohler changed the title Improve speed in sanitize through string.builder filters/builtin: improve speed in sanitize in StripQuery Filter Feb 16, 2024
@trkohler trkohler changed the title filters/builtin: improve speed in sanitize in StripQuery Filter filters/builtin: minor performance optimization for sanitize in StripQuery Filter Feb 16, 2024
@trkohler
Copy link
Member Author

I attached benchmark results in last commit (afdd426)

I have also tried to use AppendQuoteToASCII, but it resulted in some failed tests. And when I do atempts to fix it properly, there are more allocations happening and it degrades benchmark result. Probably, I use it incorrectly... For now, I let the line be unchanged.

I've also re-wrote description pointing out my motivation for a contribution. I'm sorry if previously it was thought as some issue with time-pressure. It is not pressuring at all, it's just my attempt to do an enhancement on some small part of source code. I thought that if it would succeed, I can try something bigger next.

@trkohler trkohler marked this pull request as ready for review February 16, 2024 18:47
@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Feb 19, 2024

You'd need to git commit --amend -s to add DCO to the last commit.
Please include benchstat results (without individual benchmark outputs) to the last commit and PR description to demonstrate the improvement .

@trkohler trkohler force-pushed the setup-proper-benchmark branch 2 times, most recently from 534a4b4 to a3265f1 Compare February 21, 2024 10:34
This commit introduces a benchmark for the sanitize function in the stripQuery filter.

Signed-off-by: Troy Kohler <troyalekson@gmail.com>
@trkohler trkohler force-pushed the setup-proper-benchmark branch 2 times, most recently from 69d9e0e to 1bd4760 Compare February 22, 2024 16:33
pkg: github.com/zalando/skipper/filters/builtin
           │   HEAD~1    │                HEAD                │
           │   sec/op    │   sec/op     vs base               │
Sanitize-8   802.1n ± 0%   748.3n ± 0%  -6.71% (p=0.000 n=10)

           │   HEAD~1   │                HEAD                │
           │    B/op    │    B/op     vs base                │
Sanitize-8   472.0 ± 0%   192.0 ± 0%  -59.32% (p=0.000 n=10)

           │   HEAD~1   │               HEAD                │
           │ allocs/op  │ allocs/op   vs base               │
Sanitize-8   15.00 ± 0%   14.00 ± 0%  -6.67% (p=0.000 n=10)

Signed-off-by: Troy Kohler <troyalekson@gmail.com>
@trkohler trkohler force-pushed the setup-proper-benchmark branch from 1bd4760 to 1d3c012 Compare February 22, 2024 16:33
func BenchmarkSanitize(b *testing.B) {
piece := "query=cXVlcnkgUGRwKCRjb25maWdTa3U6IElEIS&variables=%7B%0A%20%20%20%20%22beautyColorImageWidth%22:%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20200,%0A%20%20%20%20%22portraitGalleryWidth%22:%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20828,%0A%20%20%20%20%22segmentedBannerHeaderLogoWidth%22:%20%20%20%20%20%20%20%20%20%20%20%20%20%2084,%0A%20%20%20%20%22shouldIncludeCtaTrackingKey%22:%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20false,%0A%20%20%20%20%22shouldIncludeFlagInfo%22:%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20false,%0A%20%20%20%20%22shouldIncludeHistogramValues%22:%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20false,%0A%20%20%20%20%22shouldIncludeOfferSelectionValues%22:%20%20%20%20%20%20%20%20%20%20%20true,%0A%20%20%20%20%22shouldIncludeOmnibusConfigModeChanges%22:%20%20%20%20%20%20%20false,%0A%20%20%20%20%22shouldIncludeOmnibusPriceLabelChanges%22:%20%20%20%20%20%20%20false,&apiEndpoint=https%253A%252F%252Fmodified%252Fsecret%252Fgraphql%252Fsecret&frontendType=secret&zalandoFeature=secret"
q := strings.Repeat(piece, 2)
v, e := url.ParseQuery(q)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use err instead of e like elsewhere.

@AlexanderYastrebov
Copy link
Member

👍

1 similar comment
@trkohler
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit e7dd553 into zalando:master Feb 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor no risk changes, for example new filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants