[go] net/url: reduce allocs in Encode
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
net/url: reduce allocs in Encode
This change adds benchmarks for Encode and reverts what CL 617356 did in
this package. In this case, pre-sizing the slice in which we collect the
keys and then sorting it is indeed more performant than relying on a
combination of maps.Keys and slices.Sorted.
Here are some benchmark results:
goos: darwin
goarch: amd64
pkg: net/url
cpu: Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
│ old │ new │
│ sec/op │ sec/op vs base │
EncodeQuery-8 1.719µ ± 1% 1.055µ ± 0% -38.63% (p=0.000 n=20)
│ old │ new │
│ B/op │ B/op vs base │
EncodeQuery-8 576.0 ± 0% 256.0 ± 0% -55.56% (p=0.000 n=20)
│ old │ new │
│ allocs/op │ allocs/op vs base │
EncodeQuery-8 25.00 ± 0% 11.00 ± 0% -56.00% (p=0.000 n=20)
Change diff
diff --git a/src/net/url/url.go b/src/net/url/url.go
index a697548..e96542d 100644
--- a/src/net/url/url.go
+++ b/src/net/url/url.go
@@ -15,7 +15,6 @@
import (
"errors"
"fmt"
- "maps"
"net/netip"
"path"
"slices"
@@ -1048,7 +1047,14 @@
return ""
}
var buf strings.Builder
- for _, k := range slices.Sorted(maps.Keys(v)) {
+ // To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),
+ // we pre-size the slice in which we collect v's keys and then sort it.
+ keys := make([]string, 0, len(v))
+ for k := range v {
+ keys = append(keys, k)
+ }
+ slices.Sort(keys)
+ for _, k := range keys {
vs := v[k]
keyEscaped := QueryEscape(k)
for _, v := range vs {
diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go
index a7543d6..bb9811d 100644
--- a/src/net/url/url_test.go
+++ b/src/net/url/url_test.go
@@ -1101,6 +1101,17 @@
}
}
+func BenchmarkEncodeQuery(b *testing.B) {
+ b.ReportAllocs()
+ for range b.N {
+ for _, tt := range encodeQueryTests {
+ strSink = tt.m.Encode()
+ }
+ }
+}
+
+var strSink string
+
var resolvePathTests = []struct {
base, ref, expected string
}{
Change information
- M src/net/url/url.go
- M src/net/url/url_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Acknowledged
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jes Cok (Gerrit)
Jes Cok added 1 comment![]( "Open in Gerrit")
// To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),Looks like it's reverting https://go-review.googlesource.com/c/go/+/617356/3/src/net/url/url.go#b1007.
Just curious once it's submitted for performance, should the other files also be reverted?
Related details
- Damien Neil
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
// To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),Looks like it's reverting https://go-review.googlesource.com/c/go/+/617356/3/src/net/url/url.go#b1007.
Just curious once it's submitted for performance, should the other files also be reverted?
I must admit I was focused on net/url and I haven't studied the rest of CL 617356 yet. I do suspect that some of those other changes should also be reverted. Hmmm... Give me some time; I might broaden the scope of this CL to revert what should be.
Sean Liao (Gerrit)
Sean Liao added 1 comment![]( "Open in Gerrit")
strSink = tt.m.Encode()do you need the sink if you switch to b.Loop() ?
Related details
- jub0bs
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
strSink = tt.m.Encode()do you need the sink if you switch to b.Loop() ?
I'm wary of relying on `b.Loop` before #73137 gets resolved. You're right in this case, though: switching to `b.Loop` doesn't seem detrimental. Thanks!
Related details
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Damien Neil
- Russ Cox
- Sean Liao
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
// To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),jub0bsLooks like it's reverting https://go-review.googlesource.com/c/go/+/617356/3/src/net/url/url.go#b1007.
Just curious once it's submitted for performance, should the other files also be reverted?
I must admit I was focused on net/url and I haven't studied the rest of CL 617356 yet. I do suspect that some of those other changes should also be reverted. Hmmm... Give me some time; I might broaden the scope of this CL to revert what should be.
IMO, all but one of the instances of `slices.Sorted(maps.Keys(...))` introduced by CL 617356 should be reverted. The one exception is in a test, where readability matters more than performance.
However, new instances of `slices.Sorted(maps.Keys(...))` have since crept in the code base, e.g. in [net/http/server.go](https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/net/http/server.go;l=2842).
I'm not sure what the best course of action is. Perhaps this CL should only revert the instances added by CL 617356 and let the owners of the other affected packages decide what they want to do. What do you think?
Related details
- Damien Neil
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Liao (Gerrit)
Sean Liao added 1 comment![]( "Open in Gerrit")
// To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),jub0bsLooks like it's reverting https://go-review.googlesource.com/c/go/+/617356/3/src/net/url/url.go#b1007.
Just curious once it's submitted for performance, should the other files also be reverted?
jub0bsI must admit I was focused on net/url and I haven't studied the rest of CL 617356 yet. I do suspect that some of those other changes should also be reverted. Hmmm... Give me some time; I might broaden the scope of this CL to revert what should be.
IMO, all but one of the instances of `slices.Sorted(maps.Keys(...))` introduced by CL 617356 should be reverted. The one exception is in a test, where readability matters more than performance.
However, new instances of `slices.Sorted(maps.Keys(...))` have since crept in the code base, e.g. in [net/http/server.go](https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/net/http/server.go;l=2842).
I'm not sure what the best course of action is. Perhaps this CL should only revert the instances added by CL 617356 and let the owners of the other affected packages decide what they want to do. What do you think?
I think they can all stay, they're not particularly performance critical code or executed a lot of times in a regular program.
jub0bs (Gerrit)
jub0bs added 1 comment![]( "Open in Gerrit")
// To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),jub0bsLooks like it's reverting https://go-review.googlesource.com/c/go/+/617356/3/src/net/url/url.go#b1007.
Just curious once it's submitted for performance, should the other files also be reverted?
jub0bsI must admit I was focused on net/url and I haven't studied the rest of CL 617356 yet. I do suspect that some of those other changes should also be reverted. Hmmm... Give me some time; I might broaden the scope of this CL to revert what should be.
Sean LiaoIMO, all but one of the instances of `slices.Sorted(maps.Keys(...))` introduced by CL 617356 should be reverted. The one exception is in a test, where readability matters more than performance.
However, new instances of `slices.Sorted(maps.Keys(...))` have since crept in the code base, e.g. in [net/http/server.go](https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/net/http/server.go;l=2842).
I'm not sure what the best course of action is. Perhaps this CL should only revert the instances added by CL 617356 and let the owners of the other affected packages decide what they want to do. What do you think?
I think they can all stay, they're not particularly performance critical code or executed a lot of times in a regular program.
I think they can all stay [...]
Even the one in the `Encode` method? That method tends to be invoked on the server side, to modify/tidy a query string. See net/http/httputil.ReverseProxy, for instance.
Sean Liao (Gerrit)
Sean Liao added 1 comment![]( "Open in Gerrit")
// To minimize allocations, instead of using slices.Sorted(maps.Keys(v)),jub0bsLooks like it's reverting https://go-review.googlesource.com/c/go/+/617356/3/src/net/url/url.go#b1007.
Just curious once it's submitted for performance, should the other files also be reverted?
jub0bsI must admit I was focused on net/url and I haven't studied the rest of CL 617356 yet. I do suspect that some of those other changes should also be reverted. Hmmm... Give me some time; I might broaden the scope of this CL to revert what should be.
Sean LiaoIMO, all but one of the instances of `slices.Sorted(maps.Keys(...))` introduced by CL 617356 should be reverted. The one exception is in a test, where readability matters more than performance.
However, new instances of `slices.Sorted(maps.Keys(...))` have since crept in the code base, e.g. in [net/http/server.go](https://cs.opensource.google/go/go/+/refs/tags/go1.25.3:src/net/http/server.go;l=2842).
I'm not sure what the best course of action is. Perhaps this CL should only revert the instances added by CL 617356 and let the owners of the other affected packages decide what they want to do. What do you think?
jub0bsI think they can all stay, they're not particularly performance critical code or executed a lot of times in a regular program.
I think they can all stay [...]
Even the one in the `Encode` method? That method tends to be invoked on the server side, to modify/tidy a query string. See net/http/httputil.ReverseProxy, for instance.
Sorry, I meant so the others, and we merge this CL.
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
}For showing the benefit of pre-sizing, probably at least one of these should have a larger element count -- say maybe at least 10.
I think the current results in the CL commit message are probably not showing the benefit of pre-sizing, or at least, in a quick test, I seem to be able to match those allocation count results without any pre-sizing by instead just manually inlining slices.Sorted:
```diff
diff --git a/src/net/url/url.go b/src/net/url/url.go
index a697548801..a343af3b33 100644
--- a/src/net/url/url.go
+++ b/src/net/url/url.go
@@ -1048,7 +1048,9 @@ func (v Values) Encode() string {
return ""
}
var buf strings.Builder
- for _, k := range slices.Sorted(maps.Keys(v)) {
+ keys := slices.Collect(maps.Keys(v))
+ slices.Sort(keys)
+ for _, k := range keys {
vs := v[k]
keyEscaped := QueryEscape(k)
for _, v := range vs {
```
That said, just a quick test, so maybe I made a mistake, though I think it would be good to have at least one larger input in the benchmark.
Separately, I suspect there's some room for improvement on the compiler side for how it is treating the original code. I'll likely file a separate issue after I double-check a bit more.
Damien Neil (Gerrit)
Damien Neil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Damien Neil
| Code-Review | +2 |
1 comment
LGTM as-is, will wait until tomorrow before submitting in case you want to add a test case with more keys.
Related details
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |