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 ActiveRecord::Base.logger a class_attribute #42237

Merged
merged 1 commit into from
May 19, 2021

Conversation

casperisfine
Copy link
Contributor

cattr_accessor rely on class variables which has terrible
performance on long ancestor chains. See https://bugs.ruby-lang.org/issues/17763
for a detailed description of the problem.

In comparison class_attribute on ActiveRecord::Base is almost 7x
faster:

Calculating -------------------------------------
              logger      1.700M (± 0.9%) i/s -      8.667M in   5.097595s
             clogger     11.556M (± 0.9%) i/s -     58.806M in   5.089282s

Comparison:
             clogger: 11555754.2 i/s
              logger:  1700280.4 i/s - 6.80x  (± 0.00) slower

This is because ActiveRecord::Base.ancestors.size == 62.

Other cattr usages on ActiveRecord::Base should be considered too, but logger is by far the biggest hostspot, so probably the best one to start the discussion.

@eileencodes @jhawthorn @tenderlove @rafaelfranca

@jhawthorn
Copy link
Member

👍 Probably deserves a mention in the changelog. I suspect most will just be using logger as a method and won't ever be setting logger=, but you never know. Also seems plausible some folks will have used @@logger

@casperisfine
Copy link
Contributor Author

Probably deserves a mention in the changelog.

Good point, I always forget -_-.

Done.

@pixeltrix
Copy link
Contributor

Seems fine to me but I'd need @tenderlove to clarify this from the bug discussion:

I think the logger is one of the places where we depend on class variable semantics and it's unlikely to change.

@casperisfine
Copy link
Contributor Author

This quote is in opposition to "class instance variables":

class AR::Base
  class << self
    attr_accessor :logger
  end
end

Because indeed we expect MyModel.logger to work.

`cattr_accessor` rely on class variables which has terrible
performance on long ancestor chains. See https://bugs.ruby-lang.org/issues/17763
for a detailed description of the problem.

In comparison `class_attribute` on `ActiveRecord::Base` is almost 7x
faster:

```
Calculating -------------------------------------
              logger      1.700M (± 0.9%) i/s -      8.667M in   5.097595s
             clogger     11.556M (± 0.9%) i/s -     58.806M in   5.089282s

Comparison:
             clogger: 11555754.2 i/s
              logger:  1700280.4 i/s - 6.80x  (± 0.00) slower
```

This is because `ActiveRecord::Base.ancestors.size == 62`.
@byroot byroot merged commit 811f747 into rails:main May 19, 2021
@eregon
Copy link
Contributor

eregon commented May 21, 2021

Nice! I thought that change made sense too when I wrote about it in https://bugs.ruby-lang.org/issues/17763#note-7

casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 3, 2021
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.
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 10, 2021
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.
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

5 participants