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

Performance improvements to StatSerializer #156

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Oct 28, 2020

Similar to #155, this PR reduces the overhead of submitting metrics by dogstatsd-ruby.

Most of the gains came from avoiding successive string resizing performed by String#<<.

Another large gain came from replacing string.dup.gsub! with string.gsub in #formated_name:

require 'benchmark/ips'
a = "abc"
Benchmark.ips do |x|
  x.report('gsub') { a.gsub('a', 'z') }
  x.report('dup.gsub!') { a.dup.gsub!('a', 'z') }
  x.compare!
end

# Comparison:
#        gsub:  1600766.3 i/s
#   dup.gsub!:  1091421.3 i/s - 1.47x  (± 0.00) slower

A smaller gain came from removing the use of tap in two locations:

require 'benchmark/ips'
Benchmark.ips do |x|
  x.report('inline') do
    s = "abc"
    s.tr!('a', 'z')
    s
  end
  x.report('tap') do
    "abc".tap { |s| s.tr!('a', 'z') }
  end
  x.compare!
end

# Comparison:
#   inline:  4300892.6 i/s
#      tap:  3601127.4 i/s - 1.19x  (± 0.00) slower

There were no memory improvements performed.

Benchmark results

Before

Calculating -------------------------------------
             no tags     822.563k (± 1.9%) i/s -      4.159M in   5.058459s
no tags + sample rate    557.104k (± 1.7%) i/s -      2.800M in   5.027398s
           with tags     290.806k (± 1.9%) i/s -      1.474M in   5.070795s
with tags + sample rate  248.824k (± 1.9%) i/s -      1.254M in   5.040946s

Comparison:
                no tags:   822563.2 i/s
  no tags + sample rate:   557104.3 i/s - 1.48x  (± 0.00) slower
              with tags:   290805.8 i/s - 2.83x  (± 0.00) slower
with tags + sample rate:   248823.6 i/s - 3.31x  (± 0.00) slower

After

Calculating -------------------------------------
             no tags       1.098M (± 1.4%) i/s -      5.526M in   5.036068s
no tags + sample rate    707.986k (± 1.3%) i/s -      3.579M in   5.055567s
           with tags     325.535k (± 2.9%) i/s -      1.629M in   5.009379s
with tags + sample rate  275.804k (± 2.6%) i/s -      1.386M in   5.027488s

Comparison:
                no tags:  1097525.6 i/s
  no tags + sample rate:   707986.2 i/s - 1.55x  (± 0.00) slower
              with tags:   325534.7 i/s - 3.37x  (± 0.00) slower
with tags + sample rate:   275803.8 i/s - 3.98x  (± 0.00) slower

Results

Wall time % improvement
no tags 33.4%
no tags + sample rate 27.0%
with tags 11.9%
with tags + sample rate 10.8%

@marcotc
Copy link
Member Author

marcotc commented Oct 28, 2020

Another performance improvement for review when you have a chance, @kbogtob.

Copy link

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

lgtm, i think using string interpolation should help here, seems like we've preserved all the same logic and case handling

# replace reserved chars (: | @) with underscores.
f.tr!(':|@', '_')
if name.is_a?(String)
# DEV: gsub is faster than dup.gsub!

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

This is interesting given the extra allocation(?) in gsub!, do we know why?

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

(added a question about gsub! but feel free to merge IMO)

# replace reserved chars (: | @) with underscores.
f.tr!(':|@', '_')
if name.is_a?(String)
# DEV: gsub is faster than dup.gsub!
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting given the extra allocation(?) in gsub!, do we know why?

@remeh remeh merged commit eb46dea into DataDog:master Nov 16, 2020
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.

None yet

4 participants