Skip to content
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.

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

Perf: micro optimised Result column hash_row creation #12065

Merged
merged 1 commit into from Sep 3, 2013

Conversation

@SamSaffron
Copy link
Contributor

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)

index = 0
length = columns.length

while index < length

This comment has been minimized.

Copy link
@egilburg

egilburg Aug 29, 2013

Contributor

Is this significantly more performant than using columns.each_with_index?

This comment has been minimized.

# though elegant, the verbose way is much more efficient
# both time and memory wise cause it avoids a big array allocation
# this method is called a lot and needs to be micro optimised
hash = Hash.new

This comment has been minimized.

Copy link
@charliesome

charliesome Aug 29, 2013

Contributor

why not {}

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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 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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

Merge pull request #12065 from SamSaffron/result_optimisation
Perf: micro optimised Result column hash_row creation

@tenderlove tenderlove merged commit c64a001 into rails:master Sep 3, 2013

1 check passed

default The Travis CI build passed
Details

tenderlove added a commit that referenced this pull request Sep 11, 2013

Merge pull request #12065 from SamSaffron/result_optimisation
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
Projects
None yet
5 participants
You can’t perform that action at this time.