-
Notifications
You must be signed in to change notification settings - Fork 371
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
[PROF-9342] Introduce profiler workaround for Ruby Dir interruption bug #3720
Merged
ivoanjo
merged 22 commits into
master
from
ivoanjo/prof-9342-dir-interruption-workaround
Jul 2, 2024
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
b03696e
Expose methods for holding/resuming interruptions
ivoanjo aeb2db6
Bootstrap setting for controlling dir interruption workaround
ivoanjo 88f1164
Add monkey patches for affected dir class APIs
ivoanjo 3c51234
Add inline suggestion to function
ivoanjo 1721270
Add benchmark for hold/resume interruptions
ivoanjo 42c40b2
Add helper to load monkey patches (similar to our Kernel monkey patches)
ivoanjo 25a96a1
Add type signatures for dir monkey patches
ivoanjo 57e64bf
Load new file when loading profiler
ivoanjo 0e80c77
Wire up dir interruption workaround to setting
ivoanjo 577fdb0
Add test coverage for DirMonkeyPatches
ivoanjo 43a3e5e
Add comments to make Rubocop happy
ivoanjo 9152470
Add nice description to dir interruption workaround setting
ivoanjo b30ca79
Tweak spec for compatibility with Ruby 2.7
ivoanjo d6eebe9
Load `DirMonkeyPatches` before referencing it to avoid failing on JRuby
ivoanjo 30601a2
Tweak spec to remove assumption that no signals workaround is disable…
ivoanjo 9ad82fe
Make rubocop happy
ivoanjo 8099c94
Tweak naming of functions to clarify that it's signals being held
ivoanjo ba0d6b5
Avoid ruby2_keywords and instead have variants for Ruby 2 and 3
ivoanjo a6366f9
Add test to make sure we don't forget to register new profiler benchm…
ivoanjo 3501237
Add new benchmark to validate_benchmarks_spec
ivoanjo 533eeca
Merge branch 'master' into ivoanjo/prof-9342-dir-interruption-workaround
ivoanjo 259e134
Add clarification for expected behavior of yield
ivoanjo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# Used to quickly run benchmark under RSpec as part of the usual test suite, to validate it didn't bitrot | ||
VALIDATE_BENCHMARK_MODE = ENV['VALIDATE_BENCHMARK'] == 'true' | ||
|
||
return unless __FILE__ == $PROGRAM_NAME || VALIDATE_BENCHMARK_MODE | ||
|
||
require 'benchmark/ips' | ||
require 'datadog' | ||
require 'pry' | ||
require_relative 'dogstatsd_reporter' | ||
|
||
# This benchmark measures the performance of the hold/resume interruptions used by the DirMonkeyPatches | ||
class ProfilerHoldResumeInterruptions | ||
def create_profiler | ||
Datadog.configure do |c| | ||
c.profiling.enabled = true | ||
end | ||
Datadog::Profiling.wait_until_running | ||
end | ||
|
||
def run_benchmark | ||
Benchmark.ips do |x| | ||
benchmark_time = VALIDATE_BENCHMARK_MODE ? { time: 0.01, warmup: 0 } : { time: 10, warmup: 2 } | ||
x.config( | ||
**benchmark_time, | ||
suite: report_to_dogstatsd_if_enabled_via_environment_variable(benchmark_name: 'profiler_hold_resume_interruptions') | ||
) | ||
|
||
x.report("hold / resume") do | ||
Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_signals | ||
Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_signals | ||
end | ||
|
||
x.save! 'profiler_hold_resume_interruptions-results.json' unless VALIDATE_BENCHMARK_MODE | ||
x.compare! | ||
end | ||
end | ||
end | ||
|
||
puts "Current pid is #{Process.pid}" | ||
|
||
ProfilerHoldResumeInterruptions.new.instance_exec do | ||
create_profiler | ||
run_benchmark | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be more functionally named? ("what" instead of "how")
WDYT of
dir_signal_masking
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with this name because I know it will show up in the profiling flamegraph for customers.
My thinking is that it's clearer if you see "oh, why is there a datadog thing here? ah, it's monkey patching my uses of dir". Signal masking makes me think a bit more along hte lines of "what is this signal masking thing? what is masking anyway?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, interesting! I was thinking that if I saw that in a stack trace or something I would wonder "why is Datadog monkey patching dir stuff at all?". In a way "monkey patching" is how but not what/why. In a way it's also redundant with
ext
, the sort of conventional way to say "this folder contains things that extend stuff".dir_signal_fix
maybe?But at this stage that's becoming bikeshedding and certainly not a blocker. Feel free to move forward.
Adding links to upstream issues in the comment at the top of the file would help too I think (I reread it and with only the Datadog issue link it makes it like this is a Datadog issue when it's really an upstream issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the joke about naming things being hard applies here >_>. I suspect/fear just relying on
ext/
may be too subtle (e.g. when you're looking a flamegraph), but am open to changing if folks get confused about the current name.+1 I've added a link to the upstream ticket in 259e134.