Skip to content

Perf: micro optimised Result column hash_row creation #12065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2013
Merged

Perf: micro optimised Result column hash_row creation #12065

merged 1 commit into from
Sep 3, 2013
+15 −1

Conversation

SamSaffron
Copy link
Contributor

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
index = 0
length = columns.length

while index < length
Copy link
Contributor

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

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

@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

@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

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 notifications@github.com 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

@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

@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