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

Runtime Metrics: Global VM cache statistics #1680

Merged
merged 4 commits into from
Sep 14, 2021
Merged

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Sep 10, 2021

Add global_constant_state and global_method_state to ddtrace runtime metrics.

These values track how many times the global Ruby VM cache had to be rebuilt, due to changes in constant declaration or method declarations. Both values are monotonically incrementing integers.

Such changes have a large performance impact, so it's important to ensure that these values rarely change after a product application has finished warming up.

More information about method caching here: https://tenderlovemaking.com/2015/12/23/inline-caching-in-mri.html

Follow up

  • Add new metrics to Runtime Metrics dashboard in the UI

@marcotc marcotc added the core Involves Datadog core libraries label Sep 10, 2021
@marcotc marcotc self-assigned this Sep 10, 2021
@marcotc marcotc requested a review from a team September 10, 2021 22:27
sig { params(ground: Symbol, num: Integer).returns(Indexed) }
sig { params(ground: Symbol, num: Integer).void }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is my first time making changes because of sorbet. Is this the right thing to do here? @ivoanjo?

Copy link
Member

Choose a reason for hiding this comment

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

I'm... not entirely sure. Did you do this change manually, or was it a result of a sorbet autofix? Since this is provided by sorbet, I don't think we're supposed to maintain it manually... 🤔

I took a slightly different approach an in #1679 I just re-ran bundle exec srb init and sorbet marked this file as typed: ignore. (It seems like it's since been fixed upstream sorbet/sorbet-typed#372).

In that PR, I also pinned the Sorbet version so we don't run into these random "oh sorbet updated and now it doesn't like its own rbi" issues again, since this was the second time we got bitten.

So my suggestion would be to just rebase on top of that PR (or master, once merged) instead :)

@ivoanjo
Copy link
Member

ivoanjo commented Sep 13, 2021

Also for reference, https://twitter.com/tenderlove/status/1433479390583611392?s=20 is a great thread about this as well :)

Copy link
Member

@ivoanjo ivoanjo 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 great, thank you! I'm still curious about class_serial, but if we do like it we can always add it as a follow-up PR :)

Comment on lines +4 to +38
module Environment
# Reports Ruby VM cache performance statistics.
# This currently encompasses cache invalidation counters and is CRuby-specific.
#
# JRuby emulates some CRuby global cache statistics, but they are synthetic and don't
# provide actionable performance information in the same way CRuby does.
# @see https://github.com/jruby/jruby/issues/4384#issuecomment-267069314
#
# TruffleRuby does not have a global runtime cache invalidation cache.
# @see http://archive.today/2021.09.10-205702/https://medium.com/graalvm/precise-method-and-constant-invalidation-in-truffleruby-4dd56c6bac1a
module VMCache
module_function

# Global constant cache "generation" counter.
#
# Whenever a constant creation busts the global constant cache
# this value is incremented. This has a measurable performance impact
# and thus show be avoided after application warm up.
def global_constant_state
::RubyVM.stat[:global_constant_state]
end

# Global method cache "generation" counter.
#
# Whenever a method creation busts the global method cache
# this value is incremented. This has a measurable performance impact
# and thus show be avoided after application warm up.
#
# Since Ruby 3.0, the method class is kept on a per-class basis,
# largely mitigating global method cache busting. `global_method_state`
# is thus not available since Ruby 3.0.
# @see https://bugs.ruby-lang.org/issues/16614
def global_method_state
::RubyVM.stat[:global_method_state]
end
Copy link
Member

Choose a reason for hiding this comment

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

Great docs/research! I'm curious on the omission of class_serial, did you look into it and find it less useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

class_serial doesn't have a directly measurable negative performance impact on the application: it's a value that normally keeps going up on new class creation. But small class creation is a normal part of any sufficiently complex framework.
Because the fact of creating a class is in itself not harmful, I wasn't able to find a strong argument to report it, as I believe it won't be actionable. The other two method caches statistics in the PR captures any case where dynamic class/method modifications actually cause concerning issues.

Copy link
Member

Choose a reason for hiding this comment

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

But small class creation is a normal part of any sufficiently complex framework.

During request handling? I would've thought that it would "flatten out" at some point.

Happy to go with your decision; we can always revisit it if there's customer interest for that value.

@codecov-commenter
Copy link

Codecov Report

Merging #1680 (2de42e6) into master (9d86b38) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1680   +/-   ##
=======================================
  Coverage   98.31%   98.31%           
=======================================
  Files         925      927    +2     
  Lines       44564    44617   +53     
=======================================
+ Hits        43812    43865   +53     
  Misses        752      752           
Impacted Files Coverage Δ
lib/datadog/core/environment/vm_cache.rb 100.00% <100.00%> (ø)
lib/ddtrace/ext/runtime.rb 100.00% <100.00%> (ø)
lib/ddtrace/runtime/metrics.rb 96.92% <100.00%> (+0.31%) ⬆️
spec/datadog/core/environment/vm_cache_spec.rb 100.00% <100.00%> (ø)
spec/ddtrace/runtime/metrics_spec.rb 96.92% <100.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d86b38...2de42e6. Read the comment docs.

@marcotc marcotc merged commit 8d6ef89 into master Sep 14, 2021
@marcotc marcotc deleted the add-ruby-vm-metrics branch September 14, 2021 22:26
@github-actions github-actions bot added this to the 0.53.0 milestone Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants