Increase performance

rubyonrails-core
1

I propose replace all the endless loop do to while true:

NUMBER = 100_000_000

def old_slow

index = 0

loop do

break if index > NUMBER

index += 1

end

end

def new_fast

index = 0

while true

break if index > NUMBER

index += 1

end

end

Benchmark.ips do |x|

x.report(“new_fast”) { new_fast }

x.report(“old_slow”) { old_slow }

x.compare!

end

Calculating -------------------------------------

new_fast 1.000 i/100ms

old_slow 1.000 i/100ms

new_fast 0.655 (± 0.0%) i/s - 4.000 in 6.106470s

old_slow 0.259 (± 0.0%) i/s - 2.000 in 7.707258s

Comparison:

new_fast: 0.7 i/s

old_slow: 0.3 i/s - 2.52x slower

What do you think about it?

2

In general, the Rails code base wants to use Ruby idiomatically.

loop is the most succinct idiom in Ruby for those kinds of loops, see for example:

    https://github .com/rails/rails/blob/254f57ca3668398a5fcfd4f63be5d91c4c3b1cd4/actioncable /lib/action_cable/connection/stream_event_loop.rb#L66

If a Ruby programmer sees a while true there instead, generally speaking they would shake their heads a little bit. Why is this not a loop? A comment would be needed: "This while true is here for performance".

When is it OK to do a little strange thing for performance? Where it matters, not systematically across the code base. So, for example, if you change loop with while true in the previous example, probably there won't be any noticeable difference. So you just don't.

And if the gain is tiny, the cost of writing something less idiomatic, elegant, or readable is still not worth it. Because code has to be read.

You depart from this with a scalpel, precisely where it pays off.

3

Thanks for your feedback. I agree with you about use idioms for better readable code. Its cool, but Rails has many places where this not use. A lot of code can be corrected according to this, but this is not done.

And I thought, why not use it in favor of speed?

That is what I was based when wrote this message.

4

I believe what Xavier means is that you need to demonstrate at the places where you would do the replacement that the speed up is worth the readability cost. If on a single request, that code executes once, it's probably not noticeable. That's why in the benchmark you need to run it 100 million times. If you can demonstrate that the place containing the `loop` is a hotspot that gets executed many times in a request, then the optimization is probably justified. That's why it wouldn't just be a find and replace for all instances. Allen Madsen

5

Exactly.

6

Guys, thanks you! And to close subject the last question. What do you think about this replace:

loop do

break if node.equal? NULL

yield node

node = node.parent

end

to

while !node.equal? NULL

yield node

node = node.parent

end

In this case readability and speed are higher.

7

Agree, this alternative reads better, and both options return nil.

Alternatively you could use until and leave the condition positive:

    until node.equal? NULL       yield node       node = node.parent     end

Would you like to volunteer a PR? (Please /cc @fxn if you do.)

8

Oh, of course use until is better

Yes, I would create PR.

9

Xavier, do you want find another similar place for changes?

10

Xavier, do you want find another similar place for changes?

We generally do not accept cosmetic changes in GitHub (unless they are part of a regular patch) because there is too much going on in the issue tracker, but if you find one or two more cases worth having a look please do propose them and /cc me.