Optimize range iteration #1109
Optimize range iteration #1109
Conversation
|
Unfortunately our integration benchmarks to not use ranges, so there was no change there. However, this should see a reasonable speedup / memory savings in practical usage. |
|
Why the change from |
|
@ashmaroli They are aliases in many cases, but |
Ha! T.I.L. |
|
@samdoiron You can improve the readability of the reports from MemoryProfiler by passing |
|
Fixed! |
pushrax
left a comment
pushrax
left a comment
There was a problem hiding this comment.
Liquid doesn't allow iterating over ranges with floating point bounds (since Ruby doesn't), and doesn't have a way to change the step size, so this math looks all correct to me. Only issue I see is that reverse will not work.
| end | ||
|
|
||
| segment = Utils.slice_collection(collection, from, to) | ||
| segment.reverse! if @reversed |
There was a problem hiding this comment.
Range doesn't have reverse! (or reverse), could you add a test for that? To make this work, I think we may need our own range class, since reverse_each uses a temporary array. Or just convert to an array in the reverse case and keep status quo, but I think the optimization here is great and not too hard.
There was a problem hiding this comment.
range.max.downto(range.min) should also work there, no?
There was a problem hiding this comment.
Hm, actually that would make empty? checking difficult. I'll throw together a custom enumerator.
|
A quick note, since I haven't been able to focus on this the last few days: this will require a concurrent change in Liquid C, which directly generates Range objects. I hope to have a separate PR there to update this behaviour soon. |
See Shopify/liquid#1109 for further context.
See Shopify/liquid#1109 for further context.
See Shopify/liquid#1109 for further context.
samdoiron
force-pushed
the
range-optimization
branch
2 times, most recently
from
b3d0be6 to
11b31e4
Compare
For loops support a range syntax for iterating a fixed number of times,
which looks like this.
```liquid
{% for i in range (1..100) %}
...
{% endfor %}
```
Previously, we converted these ranges to arrays using `to_a`, which
initialized an array containing each number in the range. Since all we
use these ranges for is iteration, this is far less efficient than
using a range iterator.
Doing this means that iterating over ranges now takes O(1) rather than O(n)
memory. See the PR for more benchmarks.
* Remove to_a cast on ranges
* Add ReversableRange iterator
* Add custom range-specific slicing logic
samdoiron
force-pushed
the
range-optimization
branch
from
11b31e4 to
fd5e3e8
Compare
|
@samdoiron I just opened up a PR which is a possible match to this for the liquid-c side. Shopify/liquid-c#48 |
|
I've just realised you already did one Shopify/liquid-c#46 Oh well I guess either implementation is an option |
|
👍 I'm happy to go with yours. |
|
@pushrax can we get your feedback on this and the approach for liquid-c. |
| @@ -0,0 +1,77 @@ | |||
| module Liquid | |||
| class ReversableRange | |||
There was a problem hiding this comment.
Reversible is a more correct spelling I think.
| @@ -0,0 +1,77 @@ | |||
| module Liquid | |||
| class ReversableRange | |||
| include Enumerable | |||
There was a problem hiding this comment.
We've been moving away from using Ruby builtin modules in Liquid-accessible types, as their public interface can change between Ruby versions. However, it looks okay to use Enumerable here as this is not a drop and doesn't expose its methods to Liquid.
| other.is_a?(self.class) && | ||
| other.min == min && | ||
| other.max == max && | ||
| other.reversed == reversed |
There was a problem hiding this comment.
Do we care about making ranges with switched min/max and opposite reverse being equal?
|
|
||
| def each | ||
| if reversed | ||
| index = max |
There was a problem hiding this comment.
It's likely a premature optimization, but using ivars directly is a fair bit faster in general.
Warming up --------------------------------------
attr_reader 147.241k i/s - 150.359k times in 1.021174s (6.79μs/i)
ivar 247.108k i/s - 249.557k times in 1.009912s (4.05μs/i)
Calculating -------------------------------------
attr_reader 143.032k i/s - 441.723k times in 3.088285s (6.99μs/i)
ivar 255.390k i/s - 741.323k times in 2.902711s (3.92μs/i)
Comparison:
ivar: 255389.9 i/s
attr_reader: 143031.8 i/s - 1.79x slower
with
def each
if reversed
index = @max
while index >= @min
yield index
index -= 1
end
else
index = @min
while index <= @max
yield index
index += 1
end
end
endusing ReversableRange.new(1, 100)
|
I'm assuming this would also fix #1071 |
|
Yes, it would. |
6 participants
For loops support a range syntax for iterating a fixed number of times,
which looks like this.
{% for i in range (1..100) %} ... {% endfor %}Previously, we converted these ranges to arrays using
to_a, whichinitialized an array containing each number in the range. Since all we
use these ranges for is iteration, this is far less efficient than
simply keeping the ranges as ranges.
Doing this means that iterating over ranges now takes O(1) rather than O(n)
memory.
Benchmarks
TLDR
CPU: ~10% speedup in simple cases, →∞% speedup in pathological cases.
Memory: 7% less usage in simple cases, →∞% less memory in pathological cases
This comes from a change of
O(n)->O(1)in terms of memory usage.Simple iteration, CPU
Code
Before
After
Simple iteration, Memory
Code
Before
After