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

Switch to ActiveSupport::Notifications.monotonic_subscribe #831

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tgwizard
Copy link

@tgwizard tgwizard commented Nov 8, 2024

Goal

This is a small performance improvement. monotonic_subscribe behaves the same as subscribe, except that the timestamps are computed slightly more efficiently. Since we don't use the timestamps here at all, there should be no impact for Bugsnag.

Design

See this benchmark:

Benchmark.ips do |x|
  x.report("Time.now") { Time.now }
  x.report("Process.clock_gettime") { Process.clock_gettime(Process::CLOCK_MONOTONIC) }

  x.compare!
end
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) +YJIT [arm64-darwin23]
Warming up --------------------------------------
            Time.now     1.619M i/100ms
Process.clock_gettime
                         2.191M i/100ms
Calculating -------------------------------------
            Time.now     17.496M (± 1.7%) i/s   (57.15 ns/i) -     89.054M in   5.091258s
Process.clock_gettime
                         21.589M (± 2.0%) i/s   (46.32 ns/i) -    109.528M in   5.075753s

Comparison:
Process.clock_gettime: 21588547.9 i/s
            Time.now: 17496424.0 i/s - 1.23x  slower

Changeset

Testing

@mclack
Copy link

mclack commented Nov 25, 2024

Hi @tgwizard

Thanks for raising this PR with us.

This seems like a sensible change. We'll make sure to give this a full review when priorities allow.

@mclack mclack added the needs discussion Requires internal analysis/discussion label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Requires internal analysis/discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants