Skip to content

Conversation

@noelr
Copy link

@noelr noelr commented Mar 3, 2017

We had slow performance on inserts in rails 5 because column_definitions is called for each insert.

The call to column_definitions comes from query_requires_identity_insert? which calls identity_columns

Not sure what other consequences this patch has, please review. Also I'm unsure how to test.

@metaskills
Copy link
Member

  • What type of inserts?
  • Were they simple model saves or SQL generated another way?
  • Examples?

I ask because query_requires_identity_insert? is called in two places. First when using the base #execute vs #execute_insert which Rails should be calling internally now for model saves. Rails should also be passing pk as true to that method as well where the default is false because identity inserts do not normally happen. So I need to know a bit more and any info you can share helps.

Lastly, WRT testing. That is SUPER SUPER easy now with Docker based SQL Server on Linux. I'll update the docs with how to do that and post back.

@noelr
Copy link
Author

noelr commented Mar 3, 2017

First, Thank you for your fast reply and your great work on this gem!

The inserts are normal model create! and each create calls column_definitions (tested using a puts inside column_definitions)

I can run the tests but they fail, I'll look into that. But I'm unsure how to write a test that tests the new implementation of identity_columns

@noelr
Copy link
Author

noelr commented Mar 3, 2017

Example:

require 'benchmark'
Benchmark.measure do
  50.times do
    Book.create(name: "my book")
  end
end

=> @real=3.746845999998186
vs => @real=0.08944599999813363

@metaskills
Copy link
Member

metaskills commented Mar 3, 2017

Yup, I remember spending some time on this shortly after Rails v5 release. Tho I don't remember the numbers like this. I might have been looking elsewhere. Will work on this very soon.

Test/Benchmark Diff

--- a/lib/active_record/connection_adapters/sqlserver/schema_statements.rb
+++ b/lib/active_record/connection_adapters/sqlserver/schema_statements.rb
@@ -513,7 +514,11 @@ module ActiveRecord
         end
 
         def identity_columns(table_name)
-          columns(table_name).select(&:is_identity?)
+          if $SCHEMCACHE
+            schema_cache.columns(table_name).select(&:is_identity?)
+          else
+            columns(table_name).select(&:is_identity?)
+          end
         end
--- a/test/cases/scratchpad_test_sqlserver.rb
+++ b/test/cases/scratchpad_test_sqlserver.rb
@@ -1,9 +1,13 @@
 require 'cases/helper_sqlserver'
+require 'models/book'
 
 class ScratchpadTestSQLServer < ActiveRecord::TestCase
 
   it 'helps debug things' do
-    #
+    require 'benchmark'
+    puts Benchmark.realtime { 1000.times { |n| Book.create(name: n) } }
+    $SCHEMCACHE = true
+    puts Benchmark.realtime { 1000.times { |n| Book.create(name: n) } }
   end
 
 end

Test/Benchmark Diff (output)

17.683182049018797
3.5568135359790176

@metaskills
Copy link
Member

Closing for #574

@noelr
Copy link
Author

noelr commented Mar 6, 2017

Very nice - thank you! :)

@noelr noelr deleted the performance branch March 6, 2017 07:02
@metaskills
Copy link
Member

metaskills commented Mar 6, 2017

@noelr This work in in master. I hope to release that branch to v5.0.5 within the week. Cheers!

@noelr
Copy link
Author

noelr commented Mar 6, 2017

❤️

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants