Skip to content

Conversation

@ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Jul 29, 2021

Previously if you did app.config.active_support.deprecation = :silence, some work would still be done on each call to ActiveSupport::Deprecation.warn. Specifically checking the backtrace, generating the deprecation warning, and checking if the warning is disallowed.

In very hot paths, this could cause performance issues. This PR lets you turn off deprecation reporting entirely for a specific environment.

config.active_support.report_deprecations = false

^ has the same outcome as:

config.active_support.deprecation = :silence
config.active_support.disallowed_deprecation = :silence

But it will short circuit here.

@ghiculescu
Copy link
Member Author

ghiculescu commented Jul 29, 2021

Here is a benchmark:

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 "benchmark-ips"
end

require "active_support"

class MainBranch < ActiveSupport::Deprecation
end

class WithThisPR < ActiveSupport::Deprecation
  def initialize(*)
    super
    self.silenced = true
  end
end

WithThisPR.behavior = :silence
MainBranch.behavior = :silence

Benchmark.ips do |x|
  x.report("entirely silenced with this PR") { WithThisPR.warn("foo") }
  x.report("behavior = :silence on main branch") { MainBranch.warn("foo") }
  x.compare!
end
Warming up --------------------------------------
entirely silenced with this PR
                        18.693k i/100ms
behavior = :silence on main branch
                        19.373k i/100ms
Calculating -------------------------------------
entirely silenced with this PR
                        421.698k (±39.6%) i/s -      1.626M in   5.055943s
behavior = :silence on main branch
                        174.997k (±10.0%) i/s -    871.785k in   5.028267s

Comparison:
entirely silenced with this PR:   421697.8 i/s
behavior = :silence on main branch:   174996.5 i/s - 2.41x  (± 0.00) slower

@rafaelfranca
Copy link
Member

Instead of introducing a new option, can we should change the behavior of the value :silence to set all the right configs?

@rafaelfranca
Copy link
Member

Ah, but there is also the disallowed_deprecation. Yeah, I think a new option is better.

@ghiculescu ghiculescu force-pushed the deprecation-opt-out branch from 9d346da to fb24aad Compare July 29, 2021 20:57
@rails-bot rails-bot bot added the docs label Jul 29, 2021
Previously if you did `app.config.active_support.deprecation = :silence`, some work would still be done on each call to `ActiveSupport::Deprecation.warn`. Specifically [checking the backtrace](https://github.com/rails/rails/blob/12372c54828e2ac04b230cedd5cbb75181e62d29/activesupport/lib/active_support/deprecation/reporting.rb#L21), generating the [deprecation warning](https://github.com/rails/rails/blob/12372c54828e2ac04b230cedd5cbb75181e62d29/activesupport/lib/active_support/deprecation/reporting.rb#L22), and [checking if the warning is disallowed](https://github.com/rails/rails/blob/12372c54828e2ac04b230cedd5cbb75181e62d29/activesupport/lib/active_support/deprecation/reporting.rb#L23).

In very hot paths, this could cause performance issues. This PR lets you turn off deprecation reporting entirely for a specific environment.

```ruby
config.active_support.report_deprecations = false
```

^ so has the same outcome as:

```ruby
config.active_support.deprecation = :silence
config.active_support.disallowed_deprecation = :silence
```

But it will short circuit [here](https://github.com/rails/rails/blob/12372c54828e2ac04b230cedd5cbb75181e62d29/activesupport/lib/active_support/deprecation/reporting.rb#L19).
@ghiculescu ghiculescu force-pushed the deprecation-opt-out branch from fb24aad to ea3185e Compare July 29, 2021 21:00
@ghiculescu ghiculescu requested a review from rafaelfranca July 30, 2021 15:40
@rafaelfranca rafaelfranca merged commit 207da35 into rails:main Aug 6, 2021
@ghiculescu ghiculescu deleted the deprecation-opt-out branch August 6, 2021 14:06
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.

3 participants