Fix AJ wrong deprecation message on after_callbacks_if_terminated:

[Edouard-chin]

The first commit is a feature I'm adding to ActiveSupport callbacks which I use in the second commit.

Fix AJ wrong deprecation message on after_callbacks_if_terminated:

• Problem

  In some cirumstances, the deprecation message to warn that AJ won't
  run after_(enqueue/perform) callbacks when the chain is halted
  by a throw(:abort) will be thrown even though no throw(:abort)
  was thrown.

    run_callback(:foo) do
      ...
    end

  There is two possible way for the callback body to not be executed:

  1. before callback throw a abort
  2. before callback raises an error which is rescued by an
     around callback (See associated test in this commit for
     an example)

  When 2) happen we don't want to output a deprecation message,
  because what the message says isn't true and doesn't apply.

  Solution

  In order to differentiate between 1) and 2), I have added
  a halted_callback_hook which is called by ActiveSupport callback
  whenever the callback chain is halted.

cc/ @rafaelfranca @casperisfine @etiennebarrie

[rafaelfranca]

Did you measure if those conditionals will make callbacks slower?

[Edouard-chin]

Thanks for pointing this out, didn't realise how much slower this could become 😱

Warming up --------------------------------------
             without_arity_check    32.398k i/100ms
Calculating -------------------------------------
             without_arity_check  380.361k (± 3.7%) i/s -      1.911M in   5.032849s

Comparison:
             without_arity_check:   380361.4 i/s
                with_arity_check:   106834.4 i/s - 3.56x  slower

Do you have advice on how to proceed to make this change, without impacting performance ?

[Edouard-chin]

The perf penalty comes from the deprecation. Without deprecation

Warming up --------------------------------------
             without_arity_check    34.578k i/100ms
Calculating -------------------------------------
             without_arity_check    389.923k (± 2.5%) i/s -      1.971M in   5.057767s

Comparison:
             without_arity_check:                              389922.9 i/s
             with_arity_check_no_deprecation:   331838.6 i/s - 1.18x  slower

[rafaelfranca]

I'm assuming that result is from calling the callback, not defining, right?

I think we can drop the deprecation and just let the exception happen.

Or we can see if we can only show that deprecation once and cache the conditional.

[Edouard-chin]

Sorry for the delay. I have made the change to cache the conditional.

This is the benchmark result:

❯ Warming up --------------------------------------
             without_arity_check    33.638k i/100ms
Calculating -------------------------------------
             without_arity_check    398.149k (± 2.7%) i/s -      2.018M in   5.073175s

Comparison:
             without_arity_check:   398149.2 i/s
                with_arity_check:   336557.6 i/s - 1.18x  slower

I'm assuming that result is from calling the callback, not defining, right?

That's correct, the change I made that affects performance is when running the callbacks.
This is the script I used

require 'bundler/setup'
require 'active_support/callbacks'
require 'active_support/deprecation'
require 'active_support/core_ext/object/blank'
require 'benchmark/ips'

class A
  include ActiveSupport::Callbacks

  define_callbacks(:save)
  set_callback(:save, :before) { throw(:abort) }

  def halted_callback_hook(filter)
  end

  def run
    run_callbacks(:save) do
    end
  end
end

ActiveSupport::Deprecation.silence do
  a = A.new

  Benchmark.ips do |x|
    x.report('with') do
      a.run
    end

    x.report('without') do
      a.run
    end

    x.hold! 'temp_results'
    x.compare!
  end
end

[rafaelfranca]

Callbacks are hotspot. I'm not sure we should add 20% overhead just to add a deprecation. I think it is better to document this breaking change in the CHANGELOG and release notes.

[Edouard-chin]

Ok sounds good 👍 . It's pretty uncommon to redefine halted_callback_hook so hopefully it shouldn't affect too many users