Skip to content

Perf: micro optimised Result column hash_row creation#12065

Merged
tenderlove merged 1 commit intorails:masterfrom
SamSaffron:result_optimisation
Sep 3, 2013
Merged

Perf: micro optimised Result column hash_row creation#12065
tenderlove merged 1 commit intorails:masterfrom
SamSaffron:result_optimisation

Conversation

@SamSaffron
Copy link
Contributor

@SamSaffron SamSaffron commented Aug 29, 2013

This does replace rather elegant code with more verbose code, but for good reason.

With this change applied array allocation in AR is heavily reduced:

For a Discourse sample query I am seeing

Before:

T_ARRAY 10812
T_STRING 8891
T_HASH 1703
T_OBJECT 946
T_NODE 779
T_DATA 417
T_STRUCT 128
T_MATCH 15
T_REGEXP 5
T_CLASS 3

After:

T_STRING 8891
T_ARRAY 6553
T_HASH 1703
T_OBJECT 946
T_NODE 779
T_DATA 417
T_STRUCT 128
T_MATCH 15
T_REGEXP 5
T_CLASS 3

So T_ARRAY allocations are reduced from by almost half. A micro bench shows this is also faster cpu wise.

require 'benchmark'

def zip_it(array1,array2)
  Hash[array1.zip(array2)]
end


def naive(array1,array2)
  hash = Hash.new

  index = 0
  length = array1.length
  while index < length
    hash[array1[index]] = array2[index]
    index+=1
  end

  hash
end


[2,20,40,60,100].each do |i|

  array1 = (0..i).to_a
  array2 = (0..i).to_a

  puts
  puts "Benching #{i} elements"
  puts


  Benchmark.bmbm do |x|
    x.report("zip #{i}") do
      100000.times{zip_it(array1,array2)}
    end

    x.report("naive #{i}") do
      100000.times{naive(array1,array2)}
    end
  end

end

Results:

sam@ubuntu:~/Source/play$ ruby fastest_arrays_to_hash.rb 

Benching 2 elements

Rehearsal -------------------------------------------
zip 2     0.070000   0.000000   0.070000 (  0.074535)
naive 2   0.050000   0.010000   0.060000 (  0.057374)
---------------------------------- total: 0.130000sec

              user     system      total        real
zip 2     0.060000   0.000000   0.060000 (  0.066469)
naive 2   0.060000   0.000000   0.060000 (  0.057196)

Benching 20 elements

Rehearsal --------------------------------------------
zip 20     0.380000   0.000000   0.380000 (  0.383691)
naive 20   0.350000   0.000000   0.350000 (  0.354300)
----------------------------------- total: 0.730000sec

               user     system      total        real
zip 20     0.380000   0.000000   0.380000 (  0.389739)
naive 20   0.350000   0.000000   0.350000 (  0.347917)

Benching 40 elements

Rehearsal --------------------------------------------
zip 40     0.720000   0.000000   0.720000 (  0.736159)
naive 40   0.660000   0.010000   0.670000 (  0.670283)
----------------------------------- total: 1.390000sec

               user     system      total        real
zip 40     0.720000   0.000000   0.720000 (  0.722527)
naive 40   0.660000   0.000000   0.660000 (  0.664174)

Benching 60 elements

Rehearsal --------------------------------------------
zip 60     1.110000   0.000000   1.110000 (  1.116779)
naive 60   0.780000   0.080000   0.860000 (  0.870799)
----------------------------------- total: 1.970000sec

               user     system      total        real
zip 60     0.870000   0.000000   0.870000 (  0.872703)
naive 60   0.710000   0.040000   0.750000 (  0.745715)

Benching 100 elements

Rehearsal ---------------------------------------------
zip 100     1.700000   0.000000   1.700000 (  1.712397)
naive 100   1.280000   0.060000   1.340000 (  1.341964)
------------------------------------ total: 3.040000sec

                user     system      total        real
zip 100     1.490000   0.000000   1.490000 (  1.502800)
naive 100   1.240000   0.000000   1.240000 (  1.259484)

egilburg
egilburg reviewed Aug 29, 2013
View reviewed changes
activerecord/lib/active_record/result.rb
Copy link
Contributor

@egilburg egilburg Aug 29, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this significantly more performant than using columns.each_with_index?

Copy link
Member

@carlosantoniodasilva carlosantoniodasilva Sep 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently: https://gist.github.com/carlosantoniodasilva/6428702

@SamSaffron
Copy link
Contributor Author

SamSaffron commented Aug 29, 2013

@egilburg I tried pretty much everything, nothing is as fast as the naive implementation

see:

https://gist.github.com/SamSaffron/6374131

@SamSaffron
Copy link
Contributor Author

SamSaffron commented Aug 30, 2013

@tenderlove one thing we can think of is returning a proxy object from [] etc. That way you hold a single hash that maps column names to position in array and reuse it for all rows.

It would be a pretty strong optimisation for the massive amount of over-querying rails apps tend to do. Quite often I see 50 columns selected even though only a handful are used.

@tenderlove
Copy link
Member

tenderlove commented Aug 30, 2013

Sam, that sounds like a good idea. Can you try it? I like the idea, and I can't think why it would work.

If it doesn't work, we can just pull in this optimization anyway.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Aug 29, 2013, at 5:18 PM, Sam [email protected] wrote:

@tenderlove one thing we can think of is returning a proxy object from [] etc. That way you hold a single hash that maps column names to position in array and reuse it for all rows.

It would be a pretty strong optimisation for the massive amount of over-querying rails apps tend to do. Quite often I see 50 columns selected even though only a handful are used.

�
Reply to this email directly or view it on GitHub.

@SamSaffron
Copy link
Contributor Author

SamSaffron commented Sep 3, 2013

@tenderlove I would like to get to it, but it may take a bit, recommend we pull this fix in as-is into Rails 4, since it is super safe. I am concerned changing the classes Result returns is going to need much more regression testing.

@tenderlove
Copy link
Member

tenderlove commented Sep 3, 2013

@SamSaffron sounds good. I'll merge this and backport!

tenderlove added a commit that referenced this pull request Sep 3, 2013
@tenderlove
Merge pull request #12065 from SamSaffron/result_optimisation
c64a001 
Perf: micro optimised Result column hash_row creation
@tenderlove tenderlove merged commit c64a001 into rails:master Sep 3, 2013
@SamSaffron SamSaffron mentioned this pull request Sep 10, 2013
Merged
tenderlove added a commit that referenced this pull request Sep 11, 2013
@tenderlove
Merge pull request #12065 from SamSaffron/result_optimisation
e1314d8 
Perf: micro optimised Result column hash_row creation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@SamSaffron @tenderlove @carlosantoniodasilva @haileys @egilburg