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

Iterate over combined hook slices instead of iterating using two loops #810

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

brandur
Copy link
Contributor

@brandur brandur commented 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.

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.
Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for testing both approaches.

@brandur
Copy link
Contributor Author

brandur commented Mar 18, 2025

NP. Thanks!

@brandur brandur merged commit 402f301 into master Mar 18, 2025
10 checks passed
@brandur brandur deleted the brandur-append-hooks branch March 18, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants