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

Define adapter type maps statically when possible #42773

Merged
merged 1 commit into from Jul 13, 2021

Conversation

casperisfine
Copy link
Contributor

Each type map can use a non trivial amount of memory (over 10KiB
in our app).

Currently each connection build its own type map from scratch, but
except for postgresql which has extension types, all connections
end up with the same maps.

So the more connections you have the more memory it wastes.

By defining the type map statically for MySQL and SQLite3 connections
we save some memory, share caches, and allow that memory to be handled
by Copy on Write for forking setups.

Each type map can use a non trivial amount of memory (over 10KiB
in our app).

Currently each connection build its own type map from scratch, but
except for postgresql which has extension types, all connections
end up with the same maps.

So the more connections you have the more memory it wastes.

By defining the type map statically for MySQL and SQLite3 connections
we save some memory, share caches, and allow that memory to be handled
by Copy on Write for forking setups.
@casperisfine
Copy link
Contributor Author

over 10KiB in our app

So after digging a bit more, this might be a bit over reported since quite a few values are shared (e.g. frozen strings).

But still, after loading a good chunks of our models:

>> Shop.connection.send(:type_map).instance_variable_get(:@cache).size
=> 22
>> DeepMemsize.memsize_of(Concurrent::Map.new.tap { |m| m[1] = 2})
=> 280
>> DeepMemsize.memsize_of(ActiveModel::Type::Boolean.new)
=> 40
>> (280 + 40) * 22
=> 7040
>> ApplicationRecord.connection.send(:type_map).instance_variable_get(:@mapping).size
=> 35
>> ObjectSpace.memsize_of(ApplicationRecord.connection.send(:type_map).instance_variable_get(:@mapping))
=> 1760

So we're not that far off, but clearly a good part of that is the Concurrent::Map as values, which is only needed for postgress, so we could severely reduce that overhead as well.

@casperisfine
Copy link
Contributor Author

a good part of that is the Concurrent::Map as values

Which also begs the question of why we were even using these since each connection had its own private type_map, so no concurrency was expected.

@casperisfine
Copy link
Contributor Author

cc @yahonda since you seem to be the maintainer of https://github.com/rsim/oracle-enhanced, and I think this PR breaks it.

cc @wpolicarpo since you seem to be the maintainer of https://github.com/rails-sqlserver/activerecord-sqlserver-adapter. I'm not sure this PR breaks it, but I'd recommend applying a similar change in your gem.

not sure if there are other popular 3rd party adapters I should notify.

@yahonda
Copy link
Member

yahonda commented Jul 13, 2021

Thanks for the heads-up.

@wpolicarpo
Copy link

Nice. Thanks for letting us know.

@byroot byroot merged commit 88ec15b into rails:main Jul 13, 2021
@chrisbloom7
Copy link
Contributor

chrisbloom7 commented Jul 14, 2021

We had a few old v4.2 migrations that referenced a :bool column type instead of a :boolean. Both columns have since been deleted so we can't check if they actually mapped successfully to boolean fields, and we don't think we had any custom map types in place at the time. Noting it here because this change could lead to no block given (yield) errors if old migrations reference a column mapping type that has since been deprecated or removed. Since these migrations were ultimately no-ops because the columns no longer exist we simple updated those erring column types to :boolean to get around this.

@byroot
Copy link
Member

byroot commented Jul 14, 2021

@chrisbloom7 I'm afraid I dont' quite understand what you are referring to. Would you have some more detailed example, or just point to the part of this patch you think introduced the breakage?

@chrisbloom7
Copy link
Contributor

chrisbloom7 commented Jul 14, 2021

@byroot we had a migration that looked like this:

class AddFooToBar < ActiveRecord::Migration[4.2]
  def change
    add_column :bar, :foo, :bool, default: false
  end
end

After upgrading to a version of edge rails that included this change, that migration started failing in schema test builds that we run. It erred with no block given (yield) and pointed to this line (which is from the followup PR #42776). This seems to only occur when a :default option is specified, because then it needs to use the TypeMap to cast the default value from Ruby to SQL, and there is no default type map for :bool. It's possible this problem is specific to MySQL adapters.

@chrisbloom7
Copy link
Contributor

Interestingly, :bool works fine as a type in a new v7.0 migration as long as no :default option is provided. Again, this could be specific to MySQL.

@byroot
Copy link
Member

byroot commented Jul 14, 2021

Thank you for the extra info. I'll try to get to the bottom of this next week (I'm mostly AFK until Monday), unless someone beats me to it.

@composerinteralia
Copy link
Member

composerinteralia commented Jul 14, 2021

I was able to get a reproduction script for this. There seem to be three conditions that cause this to fail:

  • The TypeMap has a parent (like in the mysql2 TYPE_MAP_WITH_BOOLEAN)
  • The type referenced is not in the TypeMap
  • The migration includes a default value (which we need to cast to a SQL value via the TypeMap)

To avoid setting up mysql2 in this reproduction script, I patched the sqlite3 adapter to have a parent TypeMap

Reproduction Script
# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :payments, force: true
end

# Wrap TypeMap in a parent so it behaves roughly like the mysql2 TYPE_MAP_WITH_BOOLEAN
# https://github.com/rails/rails/blob/88ec15b850790a06bd8dcfe59ca05d865f458c7c/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L624
class ActiveRecord::ConnectionAdapters::SQLite3Adapter
  private

  def type_map
    ActiveRecord::Type::TypeMap.new(super)
  end
end

class Payment < ActiveRecord::Base
end

class ChangeAmountToAddScale < ActiveRecord::Migration[7.0]
  def change
    add_column :payments, :paid, :bool, default: false
  end
end

class BugTest < Minitest::Test
  def test_migration_up
    ChangeAmountToAddScale.migrate(:up)
  end
end

@byroot
Copy link
Member

byroot commented Jul 14, 2021

@composerinteralia thank you!

@composerinteralia
Copy link
Member

Opened #42786

byroot pushed a commit that referenced this pull request Jul 14, 2021
#42773 introduced a regression where
looking up an unregistered type on a TypeMap with a parent (like
[mysql2 TYPE_MAP_WITH_BOOLEAN](https://github.com/rails/rails/blob/88ec15b850790a06bd8dcfe59ca05d865f458c7c/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L618)
would cause a `LocalJumpError`

This commit fixes the error by forwarding the default block when
fetching from the parent TypeMap.

Co-authored-by: Chris Bloom <chrisbloom7@gmail.com>
yahonda added a commit to yahonda/ruby-plsql that referenced this pull request Aug 5, 2021
mgrunberg added a commit to yellowspot/activerecord-sqlserver-adapter that referenced this pull request Dec 27, 2021
mgrunberg added a commit to yellowspot/activerecord-sqlserver-adapter that referenced this pull request Dec 31, 2021
osanay added a commit to osanay/armg that referenced this pull request May 27, 2022
In ActiveRecord 7, the `initialize_type_map` method has been moved to
class method by rails/rails#42773 and armg no
longer works correctly.

Therefore, I have added a monkey patch to the `type_map` method.
Because ActiveRecord 7 defines the type map statically when loaded,
monkey patching class method does not make sense.

https://github.com/rails/rails/blob/v7.0.3/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L616-L619

The `type_map` method is defined in the base class
`ActiveRecord::ConnectionAdapters::AbstractAdapter`. It is the same for
all versions of ActiveRecord supported by armg. And this fix works in
any version.
osanay added a commit to osanay/armg that referenced this pull request Jun 15, 2022
In ActiveRecord 7, the `initialize_type_map` method has been moved to
class method by rails/rails#42773 and armg no
longer works correctly.

Therefore, I have added a monkey patch to the `type_map` method.
Because ActiveRecord 7 defines the type map statically when loaded,
monkey patching class method does not make sense.

https://github.com/rails/rails/blob/v7.0.3/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L616-L619

The `type_map` method is defined in the base class
`ActiveRecord::ConnectionAdapters::AbstractAdapter`. It is the same for
all versions of ActiveRecord supported by armg. And this fix works in
any version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants