Permalink
Browse files

Execute less operations

  • Loading branch information...
1 parent 1f06652 commit 994a1c2a4747efcca3c6278c119096d93f793da1 @spastorino spastorino committed Nov 9, 2010
Showing with 1 addition and 2 deletions.
  1. +1 −2 activerecord/lib/active_record/associations/association_collection.rb
View
3 activerecord/lib/active_record/associations/association_collection.rb
@@ -545,8 +545,7 @@ def ensure_owner_is_not_new
def fetch_first_or_last_using_find?(args)
args.first.kind_of?(Hash) || !(loaded? || !@owner.persisted? || @reflection.options[:finder_sql] ||
- @target.any? { |record| !record.persisted? } || args.first.kind_of?(Integer))
- # TODO - would prefer @target.none? { |r| r.persisted? }
+ !@target.all? { |record| record.persisted? } || args.first.kind_of?(Integer))
end
def include_in_memory?(record)

16 comments on commit 994a1c2

@dchelimsky

I'm not sure that's any fewer operations. Depends on the conditions of the records. Consider

[1,2,3].any? { |n| n < 4 } # exits after the first iteration
[1,2,3].all? { |n| n > 0 } # exits after three iterations

Whereas

[1,2,3].any? { |n| n > 4 } # exits after three iterations
[1,2,3].all? { |n| n < 0 } # exits after first iterations

Two questions: is it provable (or even reasonable to assume) that one set of conditions is more likely than the other? If not, is one or the other more expressive? I personally prefer @target.any? over !@target.all?. But that's all I'm going to say about it! Carry on :)

@spastorino
Ruby on Rails member

David: what you said is right for your particular case but not in the code I changed. There we have exactly the same amount of iterations.
Look at this ...

rbx-head > arr = [true, true, false, true, true]
 => [true, true, false, true, true] 
rbx-head > arr.any? { |e| p e; !e }
true
true
false
 => true 
rbx-head > !arr.all? { |e| p e; e }
true
true
false
 => true 
rbx-head > arr = [false, false, true, false, false]
 => [false, false, true, false, false] 
rbx-head > arr.any? { |e| p e; !e }
false
 => true 
rbx-head > !arr.all? { |e| p e; e }
false
 => true

The win in my change is that I'm not doing ! on each element, just once.

@dchelimsky
require 'benchmark'

$array = [false, false, true, false, false]

Benchmark.benchmark do |bm|
  bm.report do
    1000000.times do
      $array.any? {|e| !e}
    end
  end
end

Benchmark.benchmark do |bm|
  bm.report do
    1000000.times do
      !$array.all? {|e| e}
    end
  end
end
$ ruby example.rb 
  1.020000   0.000000   1.020000 (  1.018434)
  1.020000   0.000000   1.020000 (  1.023339)

$ ruby example.rb 
  1.010000   0.000000   1.010000 (  1.016594)
  1.030000   0.000000   1.030000 (  1.025111)

$ ruby example.rb 
  1.020000   0.000000   1.020000 (  1.025046)
  1.020000   0.000000   1.020000 (  1.038248)

Pretty negligible, but it looks like my version wins every time :)

@tenderlove
Ruby on Rails member

until inbox.size > 0; tenderlove.care <= 0; end

@dchelimsky

Do you mean until inbox.size == 0? Can't tell if you don't care at all, or if you'll only care when you don't have other email to read :)

@tenderlove
Ruby on Rails member

Hahaha. I mean, I don't care. ;-)

It seems like readability is most important in this case. But I don't care either way. I prefer method definitions that look like this:

def foo bar, baz
  ....
end

So obviously my aesthetics are unimportant. ;-)

@dchelimsky

If readability wins, then I vote for "target has any that are not persisted" over "not all in target are persisted" :) But, I'm really just having fun here. I will not even think about it beyond lunch, which is coming shortly.

@tenderlove
Ruby on Rails member

Damn I'm hungry. It's not even 10 yet. :'(

@spastorino
Ruby on Rails member

David,

hehe it's a nice discussion, even though tenderlove doesn't care about it :P but ...

require 'benchmark'

$array = 500.times.map { true }

Benchmark.benchmark do |bm|
  bm.report do
    1000000.times do
      $array.any? {|e| !e}
    end
  end
end

Benchmark.benchmark do |bm|
  bm.report do
    1000000.times do
      !$array.all? {|e| e}
    end
  end
end
➜  /tmp  rvm 1.8.7
➜  /tmp  ruby test.rb
  4.908582   0.011364   4.919946 (  4.802724)
  4.734593   0.008990   4.743583 (  4.770612)
➜  /tmp  rvm 1.8.7
➜  /tmp  ruby test.rb
 18.330000   0.030000  18.360000 ( 18.521579)
 16.810000   0.020000  16.830000 ( 16.996578)
➜  /tmp  rvm 1.9.2
➜  /tmp  ruby test.rb
  6.480000   0.010000   6.490000 (  6.524104)
  6.010000   0.020000   6.030000 (  6.063770)

The thing is in your test you put false at first so the amount of comparisons is the same because it just does one iteration.
When the array have to do for instance 500 iterations the comparison is between 500 !s and just 1.

Anyways I don't care at all we can change back if you want guys ;).

@fxn
Ruby on Rails member

Sure, but the reverse happens when true comes first. any? and all? can't be benchmarked against each other unless you expect some bias beforehand.

@spastorino
Ruby on Rails member

Hehehe seems like we love this kind of discussions :P. I like it :).
Xavier you're right in general terms, but this particular case of any? and all? makes them opposite.

@fxn
Ruby on Rails member

Oh you are right, I was browsing this quickly with my iPad this morning and misunderstood the motivation of the benchmarks.

@spastorino
Ruby on Rails member

I'm glad we are taking a deep look to things like that. Anyways I hope we can altogether take a deep look too to more critical things. But this kind of revisions make the code bright ;).
I think David suggestion is great about having Enumerable#none? I will add to AS and change the code to use none? so everybody will be happy :).
Thanks again David.

@josevalim
Ruby on Rails member

We already have none? in Enumerable in both Ruby 1.9 and 1.8.7.

http://ruby-doc.org/ruby-1.9/classes/Enumerable.html#M002722

@spastorino
Ruby on Rails member

Great! well I will change to that. I've checked that in http://ruby-doc.org/core/classes/Enumerable.html but this is an old version (1.8.6?). I hate that http://ruby-doc.org/core/ point to a very old version.
So David was right since the beginning.

@tenderlove
Ruby on Rails member

I suggest that we add unpersisted? for deleted records, then add notpersisted? so we don't have to use the ! operator. We should also add not_persisted? in case someone tries to call the method with an underscore. Then we should refactor that in to method missing so that if someone accidentally calls a method with extra underscores, it will just work.

Please sign in to comment.