Optimize auto_notify boolean check

[sambostock]

Goal

This attempts to optimize a check that happens every time Bugsnag.notify is called.

The current implementation checks if auto_notify is an instance of TrueClass or FalseClass (in that order). As this is a "hot path" as far as Bugsnag is concerned (runs on all notifications), we can consider applying the following optimizations:

• true and false are singleton instances of TrueClass and FalseClass, respectively. Therefore, we can rely on the slightly faster .equal? check, which relies on object identity.
• false is the default for the method, so is likely the most common usage, so we can invert the order of the check and check for false before true, to benefit from short circuiting in the more common case.

Design

This new implementation is faster in the false and non-boolean cases, and while it is slower in the true case (because of the order inversion), it is still faster than the previous approach was in the false case.

It is safe because true and false are singleton instances of TrueClass and FalseClass, respectively.

The order inversion is based on the assumption that notify is usually called with auto_notify = false, as that is the default.

Notably, the new implementation does not harm readability, and is arguably clearer.

require "benchmark/ips"
[
  false, # Default    – Probably most common case
  true,  # Override   – Probably accounts for almost all other usage
  {},    # Deprecated – Probably barely any usage
].freeze.each do |auto_notify|
  Benchmark.ips do |x|
    x.compare!
    x.report("#{auto_notify}.is_a? TrueClass or #{auto_notify}.is_a? FalseClass (status quo)") do
      auto_notify.is_a? TrueClass or auto_notify.is_a? FalseClass
    end
    x.report("false.equal? #{auto_notify} or true.equal? #{auto_notify}         (proposed)  ") do
      false.equal? auto_notify or true.equal? auto_notify
    end
  end
end

produces the following results:

Implementation	false	true	{}
auto_notify.is_a? TrueClass or auto_notify.is_a? FalseClass (status quo)	11.978M (± 0.6%) i/s	17.122M (± 1.3%) i/s	11.814M (± 0.5%) i/s
false.equal? auto_notify or true.equal? auto_notify (proposal)	17.955M (± 0.4%) i/s	13.553M (± 1.6%) i/s	13.420M (± 2.5%) i/s

Changeset

The auto_notify boolean check changes in the following ways:

• .equal? is used instead of is_a?
• The order of the check is inverted (now false is checked before true)

Testing

None of the behavior should have changed, so this does not introduce any new tests. A benchmark was used to see the difference.

[johnkiely1]

Hey @sambostock, thanks for the PR. We will review and merge as soon as priorities allow.

[imjoehaines]

Thanks @sambostock!

This is a great PR description ⇔ code change ratio 😀

The base branch was changed.

[johnkiely1]

This has been released in v6.25.2