Minor refactor of ActiveRecord::SchemaMigration #10766

Merged
merged 1 commit into from May 28, 2013

Conversation

Projects
None yet
3 participants
@kylerippey
Contributor

kylerippey commented May 26, 2013

This is a very minor refactor of the ActiveRecord::SchemaMigration class.

  • Removed reference to ActiveRecord::Base when calling table_name_prefix and table_name_suffix.
  • Override table_exists? class method to bypass schema_cache used on the ActiveRecord::Base version.
  • Switched class method definitions to preferred (class << self) style.

No tests added and no documentation modified since this is a small refactor.

Kyle Rippey
Minor refactor of ActiveRecord::SchemaMigration to remove references …
…to Base, override table_exists method, and switch to preferred style for class method definitions.
@aditya-kapoor

This comment has been minimized.

Show comment
Hide comment
@aditya-kapoor

aditya-kapoor May 28, 2013

Can we have a benchmark test for this?

Can we have a benchmark test for this?

@kylerippey

This comment has been minimized.

Show comment
Hide comment
@kylerippey

kylerippey May 28, 2013

Contributor

@aditya-kapoor Here is the benchmark I ran, using the same pattern as activerecord/examples/performance.rb: https://gist.github.com/kylerippey/5663887

I performed three runs on each branch with the following results:
master
747.8 (±7.4%) i/s - 14874 in 20.003392s
741.0 (±8.6%) i/s - 14697 in 19.993949s
768.5 (±7.4%) i/s - 15264 in 19.975956s
Average: 752.43 i/s

minor_schema_migration_refactor
744.2 (±7.9%) i/s - 14784 in 20.000572s
746.9 (±7.8%) i/s - 14839 in 19.993636s
766.9 (±7.6%) i/s - 15264 in 20.020892s
Average: 752.67 i/s

Additionally, I noticed that "SchemaMigration.table_exists?" does not currently work as expected in rails master due to the fact that the ActiveRecord::Base implementation is passed through schema_cache. This is the current behavior:

ActiveRecord::SchemaMigration.create_table
ActiveRecord::SchemaMigration.table_exists? # returns false in rails master
ActiveRecord::Base.connection.table_exists?(ActiveRecord::SchemaMigration.table_name) # returns true

This change makes table_exists? work as expected by bypassing schema_cache.

Contributor

kylerippey commented May 28, 2013

@aditya-kapoor Here is the benchmark I ran, using the same pattern as activerecord/examples/performance.rb: https://gist.github.com/kylerippey/5663887

I performed three runs on each branch with the following results:
master
747.8 (±7.4%) i/s - 14874 in 20.003392s
741.0 (±8.6%) i/s - 14697 in 19.993949s
768.5 (±7.4%) i/s - 15264 in 19.975956s
Average: 752.43 i/s

minor_schema_migration_refactor
744.2 (±7.9%) i/s - 14784 in 20.000572s
746.9 (±7.8%) i/s - 14839 in 19.993636s
766.9 (±7.6%) i/s - 15264 in 20.020892s
Average: 752.67 i/s

Additionally, I noticed that "SchemaMigration.table_exists?" does not currently work as expected in rails master due to the fact that the ActiveRecord::Base implementation is passed through schema_cache. This is the current behavior:

ActiveRecord::SchemaMigration.create_table
ActiveRecord::SchemaMigration.table_exists? # returns false in rails master
ActiveRecord::Base.connection.table_exists?(ActiveRecord::SchemaMigration.table_name) # returns true

This change makes table_exists? work as expected by bypassing schema_cache.

rafaelfranca added a commit that referenced this pull request May 28, 2013

Merge pull request #10766 from kylerippey/minor_schema_migration_refa…
…ctor

Minor refactor of ActiveRecord::SchemaMigration

@rafaelfranca rafaelfranca merged commit 059e4fb into rails:master May 28, 2013

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