(Failed) iterator experiment instead of multiple for loops #809
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Here, experiment with upgrading to Go 1.23 so we can replace a couple
hooks-related TODOs where we have two separate for loops with a single
invocation that uses an iterator to iterate two slices without an
additional slice invocation.
I'm putting this up for the sake of interest, but it unfortunately turns
out to be quite a failed experiment. The iterators work, but using them
is slower than both the previous two `for` loops, and also slower than
just allocating a new slice with the contents of the previous two and
iterating that.
At larger slice sizes, the iterator becomes marginally faster than
allocating a new slice, but only _marginally_. Using two for loops
continues to be bit a bit faster (perhaps unsurprisingly).
Anyway, opened this for general interest, but am going to close it, and
probably just replace the two for loops with a slice allocation, since
it looks nicer and it's the fastest option for small slices sizes (and
we generally expect hooks to be relatively small slices).
$ go test ./rivershared/util/sliceutil -bench=.
goos: darwin
goarch: arm64
pkg: github.com/riverqueue/river/rivershared/util/sliceutil
cpu: Apple M4
BenchmarkIterateCombined/IterateCombined-10 1481770 822.0 ns/op
BenchmarkIterateCombined/SliceAllocation-10 1617640 754.1 ns/op
BenchmarkIterateCombined/TwoForLoops-10 1560508 775.7 ns/op
BenchmarkIterateCombined/IterateCombinedLong-10 1393824 863.5 ns/op
BenchmarkIterateCombined/SliceAllocationLong-10 1399813 866.7 ns/op
BenchmarkIterateCombined/TwoForLoopsLong-10 1445377 835.2 ns/op
PASS
ok github.com/riverqueue/river/rivershared/util/sliceutil 12.272s
brandur
force-pushed
the
brandur-seq-experiment
branch
from
3a9f89e to
6561c0f
Compare
brandur
added a commit
that referenced
this pull request
Mar 18, 2025
Clean up some duplicated code in which we were previously iterating over two sets of hooks and running the same code on both collections, and I'd left a TODO to investigate the use of sequences instead to avoid a slice allocation. I ran the sequences experiment over in #809, and although I'd still defend it as a worthwhile prototype, it turns out that using sequences only becomes more efficient than allocating an extra slice once we get to fairly large slice sizes, and even there the benefit is quite marginal. The benchmarks also show that at smaller slice sizes, it's actually a tiny bit faster allocating a slice to iterate over once instead of using two for loops. This is convenient because that sure looks a lot nicer code-wise anyway, so here we switch over to that approach and remove the TODOs.
|
Closing in favor of #810. |
brandur
added a commit
that referenced
this pull request
Mar 18, 2025
#810) Clean up some duplicated code in which we were previously iterating over two sets of hooks and running the same code on both collections, and I'd left a TODO to investigate the use of sequences instead to avoid a slice allocation. I ran the sequences experiment over in #809, and although I'd still defend it as a worthwhile prototype, it turns out that using sequences only becomes more efficient than allocating an extra slice once we get to fairly large slice sizes, and even there the benefit is quite marginal. The benchmarks also show that at smaller slice sizes, it's actually a tiny bit faster allocating a slice to iterate over once instead of using two for loops. This is convenient because that sure looks a lot nicer code-wise anyway, so here we switch over to that approach and remove the TODOs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here, experiment with upgrading to Go 1.23 so we can replace a couple
hooks-related TODOs where we have two separate for loops with a single
invocation that uses an iterator to iterate two slices without an
additional slice invocation.
I'm putting this up for the sake of interest, but it unfortunately turns
out to be quite a failed experiment. The iterators work, but using them
is slower than both the previous two
forloops, and also slower thanjust allocating a new slice with the contents of the previous two and
iterating that.
At larger slice sizes, the iterator becomes marginally faster than
allocating a new slice, but only marginally. Using two for loops
continues to be bit a bit faster (perhaps unsurprisingly).
Anyway, opened this for general interest, but am going to close it, and
probably just replace the two for loops with a slice allocation, since
it looks nicer and it's the fastest option for small slices sizes (and
we generally expect hooks to be relatively small slices).