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

Small optimizations #139

Merged
merged 5 commits into from
Apr 9, 2020
Merged

Conversation

tenderlove
Copy link
Contributor

@tenderlove tenderlove commented Mar 30, 2020

Hi,

I've added a couple small optimizations. The first one removes a runtime check of a constant. We can know whether or not Process.clock_gettime is supported at file require time, so we can eliminate a conditional in time.

The other optimization eliminates object allocations inside the Telemetry class. We can (and should) use monotonic time there in order to eliminate allocations and also not be subject to clock changes. Unfortunately that file seems to be tested with Timecop, and Timecop apparently doesn't support mocking Process.clock_gettime: travisjeffery/timecop#220

The changes I made should be equivalent to the Time.now.to_i call. I will try to get the tests passing later, but maybe someone more familiar can do it too?

I used this as a benchmark:

# frozen_string_literal: true

require 'datadog/statsd'
require 'logger'
require 'benchmark/ips'
require 'stackprof'

logger = Logger.new $stderr
logger.level = Logger::INFO

thing = Datadog::Statsd.new('localhost', 1234,
                    namespace: "sample_ns",
                    sample_rate: nil,
                    tags: %w{ abc def },
                    logger: nil,
                    telemetry_flush_interval: -1,
                   )

Benchmark.ips do |x|
  x.report("time") { thing.time("foobar") { } }
end

On master, I get:

$ ruby -I lib:spec time_speed.rb
Warming up --------------------------------------
                time     4.463k i/100ms
Calculating -------------------------------------
                time     47.775k (± 6.9%) i/s -    241.002k in   5.076456s

On this branch I get:

$ ruby -I lib:spec time_speed.rb
Warming up --------------------------------------
                time     4.700k i/100ms
Calculating -------------------------------------
                time     50.365k (± 7.7%) i/s -    253.800k in   5.078403s

I also measured allocations. Here is the benchmark for allocations:

# frozen_string_literal: true

require 'datadog/statsd'
require 'logger'
require 'benchmark/ips'
require 'stackprof'

logger = Logger.new $stderr
logger.level = Logger::INFO

thing = Datadog::Statsd.new('localhost', 1234,
                    namespace: "sample_ns",
                    sample_rate: nil,
                    tags: %w{ abc def },
                    logger: nil,
                    telemetry_flush_interval: -1,
                   )

def allocs
  x = GC.stat(:total_allocated_objects)
  yield
  GC.stat(:total_allocated_objects) - x
end

p allocs { 50.times { thing.time("foobar") { } } }

On master:

$ ruby -I lib:spec time_speed.rb
1752

On this branch:

$ ruby -I lib:spec time_speed.rb
1160

I guess that's about 11 allocations saved per call to time.

We can know when the file is required whether or not
`Process.clock_gettime` is supported.  At that time, we can define a
method that calls the appropriate time handler depending on what is
supported by the current Ruby.  This reduces the overhead of the `time`
method because it doesn't have to check whether Process supports
monotonic time on every call
@kbogtob
Copy link
Contributor

kbogtob commented Apr 1, 2020

Hello @tenderlove (nice nickname btw) and thanks a lot for your contribution! I'm going to do couple tests and I will review the PR. This is actually very smart optimizations!

Copy link
Contributor

@kbogtob kbogtob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! That's really smart and useful optimizations. But could you check the CI and fix the number of allocations comparing to each ruby versions:
https://travis-ci.org/github/DataDog/dogstatsd-ruby/builds/668978123?utm_source=github_status&utm_medium=notification

lib/datadog/statsd/telemetry.rb Outdated Show resolved Hide resolved
lib/datadog/statsd.rb Show resolved Hide resolved
We can avoid allocating time objects when we need to test if it's time
to send telemetry
@kbogtob
Copy link
Contributor

kbogtob commented Apr 7, 2020

Hi @tenderlove, I have fixed the tests for you. Would you be okay for me to push to your branch so that I can update the PR? Or could you cherry-pick the commits on this branch? https://github.com/DataDog/dogstatsd-ruby/tree/tenderlove/small-optimizations
Thanks!

@tenderlove
Copy link
Contributor Author

@kbogtob I pulled your changes in. Thanks!

@kbogtob
Copy link
Contributor

kbogtob commented Apr 8, 2020

Super! I'll merge the PR right after #142.

@kbogtob kbogtob merged commit 517fd41 into DataDog:master Apr 9, 2020
@tenderlove tenderlove deleted the small-optimizations branch September 8, 2020 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants