DEBUG-2647 Run multiple Ruby micro-benchmark files #3810

p-datadog · 2024-07-25T20:47:24Z

What does this PR do?

This PR runs the profiling benchmarks, which already exist in the source tree, in addition to the tracing benchmarks.

The gem loading benchmark is moved to its own file, out of tracing file, because it applies to the entire library, not just to tracing.

This PR requires the benchmarking-platform tooling changes in https://github.com/DataDog/benchmarking-platform/pull/86.

Motivation:

I wrote some micro-benchmarks for dynamic instrumentation, but the existing benchmark runner hardcodes tracing as the only benchmark it's executing.

Changing the runner to run more than one benchmark permits executing existing profiling benchmarks.

Additional Notes:

How to test the change?

Current benchmark results: https://benchmarking.us1.prod.dog/benchmarks?projectId=5&page=1&ciJobDateStart=1719776035187&ciJobDateEnd=1722368035187&benchmarkGroupPipelineId=40500915&benchmarkGroupSha=191b99d7ee22245ead9d9b5292bfb0bab6f7d09a&benchmarkId=4574549

Unsure? Have a question? Request a review!

pr-commenter · 2024-07-25T21:01:30Z

Benchmarks

Benchmark execution time: 2024-08-05 17:15:57

Comparing candidate commit ede6784 in PR branch benchmark-gem-loading with baseline commit 4fbe269 in branch master.

Found 1 performance improvements and 1 performance regressions! Performance is the same for 21 metrics, 2 unstable metrics.

scenario:Propagation - Datadog

🟩 throughput [+805.325op/s; +883.838op/s] or [+2.468%; +2.709%]

scenario:profiler gc

🟥 throughput [-11871.186op/s; -10891.373op/s] or [-3.149%; -2.889%]

codecov-commenter · 2024-07-25T21:06:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.90%. Comparing base (8fdf5a3) to head (7aece2d).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3810   +/-   ##
=======================================
  Coverage   97.90%   97.90%           
=======================================
  Files        1261     1261           
  Lines       75620    75620           
  Branches     3706     3706           
=======================================
+ Hits        74036    74037    +1     
+ Misses       1584     1583    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ivoanjo

Left some notes but it overall LGTM 👍

ivoanjo · 2024-07-31T08:30:26Z

benchmarks/run_all.sh

+#!/bin/sh
+
+# This script is invoked by benchmarking-platform shell scripts
+# to run all of the benchmarks defined in the tracer.
+
+set -ex
+
+for file in \
+  `dirname "$0"`/tracing_trace.rb \
+  `dirname "$0"`/gem_loading.rb \
+  `dirname "$0"`/profiler_*.rb;
+do
+  bundle exec ruby "$file"
+done


So I don't think it's blocking for this PR, but I wonder if we should try to put in some kind of check for files that aren't run.

E.g. something along the lines of this check?

Otherwise, it can be a bit confusing that if anyone adds a new profiler benchmark, it gets automatically picked up, but if you add any other kind of benchmark, it doesn't?

Alternatively, should we just run every .rb file on the benchmark/ folder, and move anything that's not a benchmark to a lib/ subdirectory or something like that? (Or maybe list here the files that shouldn't be run instead of the ones that should?)

I was actually not sure what the right approach to invoking "all" benchmarks was. Let me enumerate the files explicitly in this PR and try to improve the situation later.

Move of supporting files to lib is actually part of #3787 which will be on top of this PR.

Sounds reasonable -- now there's less magic, but it's a bit more consistent 👍

ivoanjo · 2024-07-31T08:35:30Z

benchmarks/gem_loading.rb

+
+GemLoadingBenchmark.new.instance_exec do
+  benchmark_gem_loading
+end


Don't forget to update https://github.com/DataDog/dd-trace-rb/blob/master/spec/datadog/tracing/validate_benchmarks_spec.rb#L12 :)

(Technically this is not a tracing-specific thing but I think it's fine to keep it there for now? -- as long as we test it somewhere)

I added a readme to the benchmarks directory with this point as a reminder.

And moved the file to top level since it validates all benchmarks, not just tracing.

…racing-specific

p-datadog · 2024-07-31T20:46:30Z

The benchmarks are now tested in CI but the profiling ones are failing due to what looks like the C extension(s) not being built. Does that file need to run in a particular configuration?

ivoanjo · 2024-08-01T09:02:35Z

The benchmarks are now tested in CI but the profiling ones are failing due to what looks like the C extension(s) not being built. Does that file need to run in a particular configuration?

Yes:

The profiler isn't supported on macOS (*you can build and use it for our own development, but needs some manual steps) so this is why the GitHub macOS builds don't work
The profiler isn't compiled by default when running the test suite. You can see this in the Rakefile -- compile_native_extensions is only added as a dependency to profiling stuff. This is for the same reason as above in a way -- most of the team works on macOS so I didn't want to break their testing.

TBH my suggestion would be to keep the validate_benchmarks_spec.rb for profiling specs separate, and inside the profiling folder, so you don't need to care about any of the two points above ;)

* master: (52 commits) Update type signature Add TODO about spec extraction Add tiny doc explaining how to build on macOS Inline `Init_libdatadog_api` into `crashtracker.c` Tag extension builds with only major.minor, skipping Ruby patch version Gracefully handle errors when failing to load libdatadog_api in crashtracker Add env flags to skip extension and to force early failures, mirroring profiling Extract `try_loading_libdatadog` to `libdatadog_extconf_helpers` Extract `pkg_config_missing?` to `libdatadog_extconf_helpers` Extract `libdatadog_folder_*` into helper that can be shared between extensions Rubocop tweaks for extconf Minor: Actually check value of `DDTRACE_DEBUG` env variable Adopt new `datadog_ruby_common.h/.c` for profiling native extension Introduce new `datadog_ruby_common.h/.c` for common bits between extensions Successful experimental extraction of crashtracker to new module Setup libdatadog_api linking based on profiling stuff Bootstrap libdatadog_api native extension Add vaccine job Add relative flag Pin passenger version ...

ivoanjo

👍 LGTM, thanks again!

p-datadog · 2024-08-05T16:36:41Z

I found the profiler benchmark validator and have done I think what you initially requested, which was to add a validator for the library benchmarks that I split out of tracing. I also added some text to the readme to mention the various validators that need to be updated.

* master: (138 commits) DEBUG-2647 Run multiple Ruby micro-benchmark files (DataDog#3810) Update type signature Add TODO about spec extraction Add tiny doc explaining how to build on macOS Inline `Init_libdatadog_api` into `crashtracker.c` Tag extension builds with only major.minor, skipping Ruby patch version Gracefully handle errors when failing to load libdatadog_api in crashtracker Add env flags to skip extension and to force early failures, mirroring profiling Extract `try_loading_libdatadog` to `libdatadog_extconf_helpers` Extract `pkg_config_missing?` to `libdatadog_extconf_helpers` Extract `libdatadog_folder_*` into helper that can be shared between extensions Rubocop tweaks for extconf Minor: Actually check value of `DDTRACE_DEBUG` env variable Adopt new `datadog_ruby_common.h/.c` for profiling native extension Introduce new `datadog_ruby_common.h/.c` for common bits between extensions Successful experimental extraction of crashtracker to new module Setup libdatadog_api linking based on profiling stuff Bootstrap libdatadog_api native extension Add vaccine job Add relative flag ...

**What does this PR do?** This PR tweaks the configuration for benchmark output results to place result files in the current directory the benchmark gets executed from, instead of placing the results in the benchmarks directory. **Motivation:** In #3810 we standardized the results output file name in benchmarks to match the benchmark name. In that PR, we used `__FILE__` as a prefix. This changed where results are placed: where previously they were placed in the current folder where the benchmarks were run from (often the root of the repo), with this PR, they started getting placed in the benchmarks directory. This clashes with our `validate_benchmarks_spec.rb` that look for files in those directories, e.g. running a benchmark and then running the test suite will make the test suite fail which is somewhat annoying. While I could've changed the tests to filter out results files, I also find it useful to place the results where I'm executing the benchmarks from, as it makes organization easier (you just run the benchmark from where you want and you get the result there ). **Additional Notes:** N/A **How to test the change?** Run the benchmarks and confirm the results file gets placed in the current folder! :)

separate gem loading benchmark

8bec446

github-actions bot added dev/testing Involves testing processes (e.g. RSpec) and removed dev/testing Involves testing processes (e.g. RSpec) labels Jul 25, 2024

p added 2 commits July 30, 2024 14:12

run all benchmarks

80def75

use same pattern for profiler benchmark save paths (benchmarks subdir)

2840239

p-datadog changed the title ~~Separate gem loading benchmark~~ DEBUG-2647 Run multiple Ruby micro-benchmark files Jul 30, 2024

p-datadog force-pushed the benchmark-gem-loading branch from 18f8019 to 191b99d Compare July 30, 2024 18:55

p-datadog marked this pull request as ready for review July 30, 2024 19:32

p-datadog requested a review from a team as a code owner July 30, 2024 19:32

p-datadog force-pushed the benchmark-gem-loading branch from 191b99d to 2840239 Compare July 30, 2024 19:38

github-actions bot added the dev/testing Involves testing processes (e.g. RSpec) label Jul 30, 2024

ivoanjo approved these changes Jul 31, 2024

View reviewed changes

p added 3 commits July 31, 2024 16:27

explicitly list all benchmarks

d401d5b

validate all of the benchmark files for lack of errors

9015359

add readme with instructions how to add a new benchmark file

af9da85

p-datadog requested a review from a team as a code owner July 31, 2024 20:31

move benchmark validation spec to top level because it is no longer t…

9d052cb

…racing-specific

p added 2 commits August 5, 2024 10:38

reorganize benchmark validators again

42b07cd

p-datadog requested a review from a team as a code owner August 5, 2024 15:18

ivoanjo approved these changes Aug 5, 2024

View reviewed changes

rubocop

ede6784

p-datadog merged commit 00299ee into master Aug 5, 2024
182 checks passed

p-datadog deleted the benchmark-gem-loading branch August 5, 2024 18:08

github-actions bot added this to the 2.3.0 milestone Aug 5, 2024

ivoanjo mentioned this pull request Aug 8, 2024

[NO-TICKET] Don't put results in benchmarking folder directly #3833

Merged

ivoanjo mentioned this pull request Aug 15, 2024

[NO-TICKET] Finish autoformatting profiler with standardrb #3845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEBUG-2647 Run multiple Ruby micro-benchmark files #3810

DEBUG-2647 Run multiple Ruby micro-benchmark files #3810

p-datadog commented Jul 25, 2024 •

edited

Loading

pr-commenter bot commented Jul 25, 2024 •

edited

Loading

codecov-commenter commented Jul 25, 2024

ivoanjo left a comment

ivoanjo Jul 31, 2024

p-datadog Jul 31, 2024

p-datadog Jul 31, 2024

ivoanjo Aug 1, 2024

ivoanjo Jul 31, 2024

p-datadog Jul 31, 2024

p-datadog Jul 31, 2024

p-datadog commented Jul 31, 2024

ivoanjo commented Aug 1, 2024

ivoanjo left a comment

p-datadog commented Aug 5, 2024

DEBUG-2647 Run multiple Ruby micro-benchmark files #3810

DEBUG-2647 Run multiple Ruby micro-benchmark files #3810

Conversation

p-datadog commented Jul 25, 2024 • edited Loading

pr-commenter bot commented Jul 25, 2024 • edited Loading

Benchmarks

scenario:Propagation - Datadog

scenario:profiler gc

codecov-commenter commented Jul 25, 2024

Codecov Report

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo Jul 31, 2024

Choose a reason for hiding this comment

p-datadog Jul 31, 2024

Choose a reason for hiding this comment

p-datadog Jul 31, 2024

Choose a reason for hiding this comment

ivoanjo Aug 1, 2024

Choose a reason for hiding this comment

ivoanjo Jul 31, 2024

Choose a reason for hiding this comment

p-datadog Jul 31, 2024

Choose a reason for hiding this comment

p-datadog Jul 31, 2024

Choose a reason for hiding this comment

p-datadog commented Jul 31, 2024

ivoanjo commented Aug 1, 2024

ivoanjo left a comment

Choose a reason for hiding this comment

p-datadog commented Aug 5, 2024

p-datadog commented Jul 25, 2024 •

edited

Loading

pr-commenter bot commented Jul 25, 2024 •

edited

Loading