-
Notifications
You must be signed in to change notification settings - Fork 3.5k
More function clauses for Range's Enumerable.member? implementation #12282
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
Conversation
927b243
to
38909ca
Compare
38909ca
to
c5bd7f8
Compare
Hi @ryanwinchester , thanks for the PR! Membership is already optimized for ranges but you are likely paying the cost of protocol dispatch in your scripts/notebooks because protocols there are not consolidated. Can you please share your solution and let us know if adding |
https://github.com/ryanwinchester/advent-of-code-elixir/blob/master/bench/day04.exs I'm using Benchee with Update: I added |
Thanks for sharing, that helps clarify a few things. My first reaction is that the algorithms you linked are different, so I would expect the performance to be different. Can you try this?
And using it in your benchmarks to see how much faster/slower it becomes? PS: to clarify, they are different because the first version is doing at least the double of comparisons, regardless of the implementation of |
Ah yeah. You have a point. I want to know if either range completely contains the other, but using |
@josevalim updated benchmark to be more "fair" and used the
Also added more comparisons, including one without the protocol dispatch. Link: https://github.com/ryanwinchester/advent-of-code-elixir/blob/master/bench/extras/range.member.exs |
So it seems the issue is that the current member implementation is slow. We should optimize it instead. :) |
@josevalim looks good
|
Doing Advent of Code 2022, Day 4, I was using a
value in range
condition in my solution and then ran this benchmark for comparing the value againstrange.first
andrange.last
instead, and thevalue in range
version was around4.6x
slower for my usage (which has multiple range comparisons in a reduce).I think if we make a special case for when
step
is+/-1
(which is probably the most common case), then we could see some improved performance.These extra function matches should allow us to avoid
Range.size/1
call (which doesabs(div(last - first, step)) + 1
), andrem(value - first, step)
calls for our our+/-1
step ranges.Thoughts?