Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize auto_notify boolean check #774

Merged

Commits on Jan 25, 2023

  1. Optimize auto_notify boolean check

    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
    sambostock committed Jan 25, 2023
    Configuration menu
    Copy the full SHA
    90c365f View commit details
    Browse the repository at this point in the history

Commits on Feb 1, 2023

  1. Update changelog

    imjoehaines committed Feb 1, 2023
    Configuration menu
    Copy the full SHA
    f23901c View commit details
    Browse the repository at this point in the history