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

Make legacy_connection_handling a module instance variable #42442

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

casperisfine
Copy link
Contributor

Ref: #42237
Ref: https://bugs.ruby-lang.org/issues/17763

Ruby class variables are slow, and get slower the more ancestors the class has.
And it doesn't seem likely to get faster any time soon. Overall I'm under the
impression that ruby-core consider them more or less deprecated.

But in many cases where cattr_accessor is used, a class instance variable
could work just fine, and would be much faster.

For Active Record this means most of the cattr_accessor on ActiveRecord::Base
could actually be attr_accessor on ActiveRecord.

I started with legacy_connection_handling as a proof of concept.

Performance

I've ran quick benchmark

Each case is benched twice, once in a class without ancestors, once in a class with 62 ancestors (like ActiveRecord::Base).

=== class.read (Namespace::NoAncestors) ===
Warming up --------------------------------------
               cattr     1.030M i/100ms
      singleton_attr     1.738M i/100ms
Calculating -------------------------------------
               cattr     10.288M (± 0.4%) i/s -     51.483M in   5.004307s
      singleton_attr     17.078M (± 0.4%) i/s -     86.882M in   5.087522s

Comparison:
      singleton_attr: 17077667.8 i/s
               cattr: 10287971.4 i/s - 1.66x  (± 0.00) slower


=== instance.read (Namespace::NoAncestors) ===
Warming up --------------------------------------
               cattr     1.034M i/100ms
      singleton_attr     1.372M i/100ms
Calculating -------------------------------------
               cattr     10.333M (± 0.3%) i/s -     51.722M in   5.005522s
      singleton_attr     13.776M (± 0.5%) i/s -     69.976M in   5.079764s

Comparison:
      singleton_attr: 13775836.2 i/s
               cattr: 10333056.2 i/s - 1.33x  (± 0.00) slower


=== class.read (Namespace::ManyAncestors) ===
Warming up --------------------------------------
               cattr   277.307k i/100ms
      singleton_attr     1.741M i/100ms
Calculating -------------------------------------
               cattr      2.796M (± 0.5%) i/s -     14.143M in   5.059035s
      singleton_attr     17.406M (± 0.5%) i/s -     88.767M in   5.099779s

Comparison:
      singleton_attr: 17406447.1 i/s
               cattr:  2795582.8 i/s - 6.23x  (± 0.00) slower


=== instance.read (Namespace::ManyAncestors) ===
Warming up --------------------------------------
               cattr   282.302k i/100ms
      singleton_attr     1.343M i/100ms
Calculating -------------------------------------
               cattr      2.820M (± 0.6%) i/s -     14.115M in   5.005650s
      singleton_attr     13.399M (± 0.5%) i/s -     67.154M in   5.012083s

Comparison:
      singleton_attr: 13398728.1 i/s
               cattr:  2819952.0 i/s - 4.75x  (± 0.00) slower

In short it's 5 to 6 times faster than cattr_accessor, and the added benefit is that it doesn't define an instance accessor by default, which in most case is not needed, so it avoid leaking useless methods.

Followup

I just converted a single attribute, but if other agree that this is the way to go, I'd like to mass migrate similar cattr in Active Record, and possibly in other gems as well for consistency.

cc @eileencodes @jhawthorn @tenderlove @rafaelfranca

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine since the documented public API is called via the Railtie. Technically an application could be using the getter to check the value but that seems rare and unlikely to me. 👍🏼

@casperisfine
Copy link
Contributor Author

Yeah I made sure it wasn't documented.

For the followups, if some are documented we can add a delegator with a deprecation warning.

Ok, I'll try to get a green CI and start working on the other attributes.

Ref: rails#42237
Ref: https://bugs.ruby-lang.org/issues/17763

Ruby class variables are slow, and get slower the more ancestors the class has.
And it doesn't seem likely to get faster any time soon. Overall I'm under the
impression that ruby-core consider them more or less deprecated.

But in many cases where `cattr_accessor` is used, a class instance variable
could work just fine, and would be much faster.

For Active Record this means most of the `cattr_accessor` on `ActiveRecord::Base`
could actually be `attr_accessor` on `ActiveRecord`.

I started with `legacy_connection_handling` as a proof of concept.
@casperisfine casperisfine force-pushed the active-record-cattr-legacy-connection branch from ca3d38b to 8459c1c Compare June 10, 2021 13:00
@byroot byroot merged commit 9e66e5d into rails:main Jun 10, 2021
@casperisfine casperisfine deleted the active-record-cattr-legacy-connection branch June 10, 2021 13:16
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 11, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 11, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 11, 2021
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 11, 2021
jyoun-godaddy pushed a commit to jyoun-godaddy/activestorage that referenced this pull request Jul 5, 2022
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

3 participants