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

Replaced 'alias' calls with an instance variable. #47

Merged
merged 1 commit into from
May 17, 2017

Conversation

fimmtiu
Copy link
Contributor

@fimmtiu fimmtiu commented Apr 24, 2017

This fixes two problems with batch:

  1. Calling alias blows out the Ruby method cache for the current class and all its subclasses, so for performance reasons you don't want to be calling alias on a regular basis.

  2. If an exception occurred while sending the batched data, it would get stuck in batched mode.

Also fixed up a few typos in the specs for fun.

@degemer degemer added this to the Next milestone Apr 26, 2017
@degemer
Copy link
Member

degemer commented Apr 26, 2017

Hi @fimmtiu, thanks for the contribution!

I didn't know about 1, do you know in this particular case what is the impact?
Thanks for fixing 2 and the typos. 🙂

My main concern is sacrificing the performance of the non-batched send_stats over the batch one.
We don't have a performance test right now, I think we'll need one in order to decide. I unfortunately won't have time to work on this in the next few weeks, if you want (and have time!) to do it before, I'll be glad to review and talk about it, otherwise it will have to wait.

Thanks again for raising this issue!

@fimmtiu
Copy link
Contributor Author

fimmtiu commented May 10, 2017

@degemer Good suggestion about the performance test! I've written a quick benchmark script, and the results seem to indicate that this PR is about the same for most cases, and faster in the case of small batches.

On master:

dennis@concertina ~/dogstatsd-ruby$ ./performance.rb
                       user     system      total        real
regular           18.860000  19.870000  38.730000 ( 38.795239)
batched single    31.590000  23.100000  54.690000 ( 54.854231)
batched 1k         9.020000   0.520000   9.540000 (  9.551664)
batched 10k        9.070000   0.510000   9.580000 (  9.588308)

On the dont-alias-all-the-things branch:

dennis@concertina ~/dogstatsd-ruby$ ./performance.rb
                       user     system      total        real
regular           18.990000  19.410000  38.400000 ( 38.532748)
batched single    24.100000  20.680000  44.780000 ( 44.883200)
batched 1k         9.290000   0.550000   9.840000 (  9.872589)
batched 10k        9.290000   0.500000   9.790000 (  9.806697)

Here's the script I was testing with:

#!/usr/bin/env ruby

require 'benchmark'
require 'rubygems'
$: << './lib'
require 'datadog/statsd'

statsd = Datadog::Statsd.new('localhost', 8125)

n = 5_000_000
Benchmark.bm(16) do |x|
  x.report("regular") do
    n.times { statsd.increment('woo') }
  end
  x.report("batched single") do
    n.times do
      statsd.batch { |s| s.increment('woo') }
    end
  end
  x.report("batched 1k") do
    (n / 1_000).times do
      statsd.batch do |s|
        1_000.times { s.increment('woo') }
      end
    end
  end
  x.report("batched 10k") do
    (n / 10_000).times do
      statsd.batch do |s|
        10_000.times { s.increment('woo') }
      end
    end
  end
end

@degemer
Copy link
Member

degemer commented May 17, 2017

Thanks a lot for the benchmark! I re-ran it locally and obtained the same results (small improvements for small batches, equivalent everywhere else).
Moreover the code is way easier to understand this way.
Merging, this will be part of the next release (targeting end of next week).

@degemer degemer merged commit c41aa6c into DataDog:master May 17, 2017
@degemer degemer modified the milestones: 3.0.0, Next May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants