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.
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 ofTrueClass
orFalseClass
(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
andfalse
are singleton instances ofTrueClass
andFalseClass
, 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 forfalse
beforetrue
, 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 thetrue
case (because of the order inversion), it is still faster than the previous approach was in thefalse
case.It is safe because
true
andfalse
are singleton instances ofTrueClass
andFalseClass
, respectively.The order inversion is based on the assumption that
notify
is usually called withauto_notify = false
, as that is the default.Notably, the new implementation does not harm readability, and is arguably clearer.
produces the following results:
false
true
{}
auto_notify.is_a? TrueClass or auto_notify.is_a? FalseClass
(status quo)false.equal? auto_notify or true.equal? auto_notify
(proposal)Changeset
The
auto_notify
boolean check changes in the following ways:.equal?
is used instead ofis_a?
false
is checked beforetrue
)Testing
None of the behavior should have changed, so this does not introduce any new tests. A benchmark was used to see the difference.