PERF: optimise type lookup to avoid invoking procs #17643

Merged
merged 1 commit into from Nov 17, 2014

Projects

None yet

4 participants

@SamSaffron
Contributor

Mostly resolves #17641

require 'benchmark/ips'
require 'stackprof'

require File.expand_path("../../config/environment", __FILE__)

connection = ActiveRecord::Base.connection

sql = "select #{(1..10).map{|i| "#{i} #{(96+i).chr}" }.join(", ")}"
puts "SQL is #{sql}"

Benchmark.ips do |b|
  b.report("select_all") do |times|
    while times > 0
      connection.select_all(sql)
      times -= 1
    end
  end
end

cc @sgrif @tenderlove

Before:

sam@ubuntu discourse % RAILS_MASTER=1 RAILS_ENV=production ruby script/select_all_bench.rb
SQL is select 1 a, 2 b, 3 c, 4 d, 5 e, 6 f, 7 g, 8 h, 9 i, 10 j
Calculating -------------------------------------
          select_all   328.000  i/100ms
-------------------------------------------------
          select_all      3.337k (± 5.5%) i/s -     16.728k

after

SQL is select 1 a, 2 b, 3 c, 4 d, 5 e, 6 f, 7 g, 8 h, 9 i, 10 j
Calculating -------------------------------------
          select_all   507.000  i/100ms
-------------------------------------------------
          select_all      5.409k (± 5.1%) i/s -     27.378k

So we are still a touch slower than 4.1 but almost there

@sgrif sgrif commented on the diff Nov 17, 2014
...tive_record/connection_adapters/postgresql_adapter.rb
@@ -431,16 +431,22 @@ def translate_exception(exception, message)
private
def get_oid_type(oid, fmod, column_name, sql_type = '') # :nodoc:
- if !type_map.key?(oid)
+
+ result = type_map.fetch(oid, fmod, sql_type) {
+ nil
+ }
+
+ unless result
@sgrif
sgrif Nov 17, 2014 Member

Why not put this in the body of the fetch block?

@SamSaffron
SamSaffron Nov 17, 2014 Contributor

there is an odd side effect thing going there (loading fallback types) so I did not want to change the existing logic.

@sgrif
Member
sgrif commented Nov 17, 2014

Looks good to me, will merge after CI

@rafaelfranca
Member

:shipit:

@rafaelfranca rafaelfranca added this to the 4.2.0 milestone Nov 17, 2014
@sgrif sgrif merged commit ed1c3bc into rails:master Nov 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@thedarkone
Contributor

I think the newly added @cache needs to be ThreadSafe::Cache instead of a plain Hash.

@sgrif
Member
sgrif commented Nov 19, 2014

This object is not shared across multiple instances of the connection
adapter, so it will not be used across threads.

On Tue, Nov 18, 2014, 3:33 AM thedarkone notifications@github.com wrote:

I think the newly added @cache needs to be ThreadSafe::Cache instead of a
plain Hash.


Reply to this email directly or view it on GitHub
#17643 (comment).

@thedarkone
Contributor

Saw it being instantiated into a const here, that's why I was concerned.

@sgrif
Member
sgrif commented Nov 21, 2014

You're technically right, but that const is rarely used for things like SHOW CREATE TABLE, which shouldn't happen across threads. I'd rather not dig up every place it could be used though, so we should probably make that const thread safe. As long as there's no significant hit from using a thread safe cache on the object itself, I have no objections. Otherwise we can just wrap the find_type method which uses that const in a mutex.

@sgrif
Member
sgrif commented Nov 21, 2014

I'll benchmark and do one of those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment