Avoid extra allocation in String#from and #to #36180
Conversation
|
Can you improve |
Removes unnecessary Range object allocations for a significant speed-up. String#from Comparison: new: 3378594.0 i/s old: 2380129.8 i/s - 1.42x slower String#to Comparison: new: 2866175.7 i/s old: 2304406.4 i/s - 1.24x slower
jonathanhefner
force-pushed
the
optimize-string-from
branch
from
5906547
to
52498cc
Compare
jonathanhefner
changed the title
|
@kamipo Done! |
| @@ -61,7 +61,8 @@ def from(position) | |||
| # str.from(0).to(-1) # => "hello" | |||
| # str.from(1).to(-2) # => "ell" | |||
| def to(position) | |||
| self[0..position] | |||
| position = [position + length, -1].max if position < 0 | |||
There was a problem hiding this comment.
Curious what the -1 and max here is for? I could get all tests to pass with: position += size if position < 0.
There was a problem hiding this comment.
I suppose the condition does not preserve the original behavior, or hard to understand what this do.
Maybe we should more exercise that in tests.
There was a problem hiding this comment.
It is for when position + length is less than -1.
There was a problem hiding this comment.
@rafaelfranca Yes, exactly.
For example, "foo".to(-10) without the max becomes "foo"[0, -10 + 3 + 1] or "foo"[0, -6], which returns nil.
There was a problem hiding this comment.
Ah, I see! I found relying on -1 + 1 to get a 0 length for String#[] a little confusing, so I tried to change it a bit in efa2299 with extra tests.
There was a problem hiding this comment.
@kaspth I had considered that variation, because it is a bit more clear. But, I decided to use max because the extra to_s makes it ~13% slower when the position isn't negative. Of course, it's only a micro-benchmark and likely of no consequence, but since the purpose of the PR was performance, that's what I went with.
There was a problem hiding this comment.
Changed to fastest variant 71e41a5.
There was a problem hiding this comment.
@kamipo Excellent! 👍
These performance optimizations have been contributed to Active Support in rails/rails#36180.
Removes unnecessary Range object allocations for a significant speed-up.