From 90c365f35fdae4f7fd14138d49db2ee8f8c610db Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Wed, 25 Jan 2023 16:12:28 -0500 Subject: [PATCH 1/2] Optimize auto_notify boolean check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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. We can test this using the following benchmark: 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 which 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 --- lib/bugsnag.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/bugsnag.rb b/lib/bugsnag.rb index a25ed6b4..5d002a90 100644 --- a/lib/bugsnag.rb +++ b/lib/bugsnag.rb @@ -78,7 +78,7 @@ def configure(validate_api_key=true) # # Optionally accepts a block to append metadata to the yielded report. def notify(exception, auto_notify=false, &block) - unless auto_notify.is_a? TrueClass or auto_notify.is_a? FalseClass + unless false.equal? auto_notify or true.equal? auto_notify configuration.warn("Adding metadata/severity using a hash is no longer supported, please use block syntax instead") auto_notify = false end From f23901c198c99cef919a11ebfca88b260210828b Mon Sep 17 00:00:00 2001 From: Joe Haines Date: Wed, 1 Feb 2023 09:46:39 +0000 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65fe0fdf..35066185 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,14 @@ Changelog ========= +## TBD + +### Enhancements + +* Improve performance of `Bugsnag.notify` + | [#774](https://github.com/bugsnag/bugsnag-ruby/pull/774) + | [sambostock](https://github.com/sambostock) + ## v6.25.1 (5 January 2023) ### Fixes