-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
jruby9k not allowing set_trace_func without --debug, diverges from mri behavior #3096
Comments
Historically, we did not support set_trace_func and eventing which it depends on because it slows down everything even when it is not being used by a measurable amount. If you use this behavior you have to supply --debug. This is a difference with MRI, but we made a decision to give a majority better performance at the cost of users using this flag when they want this behavior. With that said... In JRuby 9000 we instrument the events as instructions now. At the moment, we do not support this but we could re-instrument event instructions on demand so that --debug is not required. This is not a trivial amount of code to support but it is possible so there is hope. Are you saynig your are seeing this many times in your app? Or perhaps once per restart? |
Ah, thanks for the info. Is the "eventing" referenced above at the java level? Also, does the performance hit from eventing take into consideration uses such as mine, where using TracePoint, Im watching for "class end", explicitly, and cancelling the watch after it hits? Well, I'm seeing it multiple times per restart, (but once it finishes loading, it stops, though things still fail to load properly since it was disabled) because I've been using it as an include in multiple places, even outside of the abstract methods use case listed above, such as in Perhaps a clearer example than the abstract methods one above, is lets say an Enum class that we want to be able to define the enum values as constants -- i.e. (and Im not saying this is the best way to do this, but for sake of illustration) class Enum
include ::Trax::Core::InheritanceHooks
after_inherited do
constants.each do |name|
#whatever logic to make it act like an enum here, but point being
#constants/methods/props of the subclass arent accessible using self.inherited
#since inherited is called immediately, before the class has finished evaluating,
#so this after inherited hook just passes scope to subclass,
#watches for the end of the current class definition,
#and after its reached, stops watching via trace.disable
end
end
end
class Category < Enum
CLOTHING = 1
SHOES = 2
end So anyways, after loading my app in jruby for the first time and seeing things break, figured Id pump the breaks on using these type of hooks further as I obviously don't want my lib to be jruby incompatible. But if it could be supported without the --debug flag, that would be super awesome. The abstract methods use case however is one where I just don't see any other (not completely wonky) way of implementing it in ruby. |
@jasonayre we will be reviewing the ability to add the events on-demand once we start work (again) on profiled optimizations. This will be after 9.0.0.0 but will happen during point releases. Once we have a system for deopt'ing back to a safe point we should be able to weave in the events and replace with a newer version. Then no more --debug needed and if you enable it soon enough then the deopt for your user code will not happen since it will be one when it originally compiles it. To answer your question, the cost is reduced when you no longer have anything registered but the code since no handlers are registered but the code which checks to see if there are handlers is still constantly being asked. So it is a bump in perf. |
I did an experiment along these lines! #5633 So the bottom line is that we can support a number of static metadata trace events (what file is running, what line is being executed, etc) pretty easily; when not used those events won't happen and will largely be no-ops. When activated, code that was previously not set up for debug events will adjust;. However, most user-facing hook usage is through MRI supports these features all the time because it doesn't really do that much optimization in the first place. Other implementations support this by basically throwing a bunch of optimizations away if anyone triggers the related event, leaving you with an app that expects I have not figured out whether the balance between always-on trace events and production performance can be mitigated just yet. For the moment we are not going to integrate that PR, but it has taught us various things. |
I have gotten a few requests about having Coverband support JRuby (see the issue Github linked above)... I finally tried to get tests and a Rails app working, and found this open issue... I believe what this issue means is that since quick reproducible details: I will try to get everything working with |
First off, I would strongly recommend against using any of the trace APIs in production. They cause overhead not just on JRuby, but on every Ruby implementation. Tracking every line event all the time should have a pretty large impact on runtime performance. Over the years we have added back some events that happen infrequently enough that they can be always-on. I'm not certain which events always fire right now, but for example class entry/exit events or raise events are infrequent as well as having enough overhead on their own that triggering the event will be of negligible impact. We are interested in making more events be always-on, but there are other problems that result: specifically, I'm not sure exactly what features you need available in production, so we should talk about that. |
So check out https://github.com/danmayer/coverband/ to learn more about the project. The project doesn't directly use The project Coverband, has been running for years, and runs in production on hundreds of projects. We use it at a very large consumer site with heavy traffic, on the primary monolith and a number of smaller microservices. We have benchmarked with it on and off and can't see meaningful performance differences even at the p95 levels. It is very well battle-tested in terms of MRI. In fact, the Cookpad team, while not using Coverband now is doing the same pattern and runs I haven't had a good opportunity to use JRuby, if you think the overhead is much more significant for JRuby, I could say that JRuby isn't supported as a platform. Is it perhaps better to make a new issue and separate the isolated cases related to |
Ok, Coverage is a bit different. Internally, both JRuby and CRuby have an "event hook" API. This API backs up Because Coverage runs entirely in "native" code (Java in our case) and it mostly listens for line events, it's the cheapest of the three. It can be largely supported without removing our optimizations, but it's still far from free; every line of code has the additional overhead of triggering an event hook, which looks up file and line and increments a counter. However, I do see that both the presentation you linked and Coverband itself uses the |
So Coverband supports both older Coverage and oneshot, oneshot is definitely faster, but Coverband helps with a lot of older legacy systems so we currently support Ruby 2.3 and up. I have measured both and honestly on a lot of older large systems even the overhead of Coverage when limited to app code, since Coverage only puts the hooks on files that are required after it, hasn't been significant in my benchmarking. The app code layer is such a small amount of called lines in a typical Rails (or Rack) app that the time spent is dwarfed by other things. This is the typical load pattern Coverband sets up.
All this being said, if JRuby only wanted to support oneshot, we could have Coverband only support JRuby via oneshot mode. I could also get everything supported, and say it requires |
Ok, so I looked into the C code that CRuby uses to implement In CRuby this is easy because they always interpret. This allows them to rewrite these instructions at any time and you don't see much performance impact, because it's all equally "slow" like an interpreter. For the same reasons JRuby doesn't support always-on coverage, the new Ruby JIT "mjit" also does not support event hooks (currently). When you enable an event hook API like Tracepoint or Coverage, JIT is immediately disabled. If this doesn't change, Coverband and oneshot_coverage will either not work or will hinder application performance even on CRuby. |
Interesting... I will have to keep an eye on that it would be sad to loose access to such interesting and cool data... but I understand why it is problematic. So far, I haven't heard from anyone trying to run apps with the |
We could also do the same thing CRuby does and disable JIT for files that have coverage enabled... but I don't think that's a good path to take either. |
FWIW I just measured JRuby running a benchmark with and without
With full Coverage enabled, this drops way down mostly due to the overhead of calling line events every time:
It's probably possible to make At this point the main reason we don't have these traces enabled for things like Coverage (which don't need a Binding) is to reduce the amount of extra code we would have to generate (which means longer startup and warmup time for apps, and possibly reduced native JIT optimization.) @enebo Brainstorming a bit here... I believe the big piece we're missing here is that we're still implementing Coverage using a global event hook. That seems like it's different in CRuby; they actually instrument the instruction sequences now and install hook events at each line. This allows them to turn those hooks off line-by-line as code is encountered. Supporting this in JRuby would basically just be a matter of having our LineNumber operation include a flag saying if it had been encountered yet. Lines encountered during interpretation would never need additional tracing; the jitted version would simply not output that logic. Lines not encountered during interpretation would use a SwitchPoint to fire exactly once and mark the line as encountered, allowing the JVM JIT to later eliminate the trace event. Since most code will execute in either our interpreter or the JVM's interpreter at least once before going to JIT, most JIT output would have no |
cool I will continue to follow this... Also, I should note, yes Generated benchmarks always have some issues, so I have also relied on NewRelic and DataDog stats on a number of production systems to ensure low-performance overhead. I mostly look at impacts on the p95 scale. If there is interest I have a series of posts on the various benchmarking efforts the project has gone through over the years. |
@headius For line that seems like it would be cheap. For posterity, I am including my original thoughts on boiling the tracing ocean. Startup interp gets all trace instrs and they more or less noop if tracing is off in interp (which will have a noticeable but hopefully small cost since we are emulating a machine). JIT will not emit any trace instrs initially. At entry to a JIT compiled unit OR when control returns (e.g. return from a call instr) we check to see if it is enabled (checkpoint) and then raise an exception. That takes us back to startup interp and we either immediately reJIT or just let it warmup again. I believe with switchpoint we could then probably have a very low overhead cost for not requiring --debug. I do naively think it will still be some cost. The obvious complexity is the same one we have with generic deoptimization...we need to continue from a particular ipc. If deoptimization existed already this would be pretty easy but there is some infrastructure needed to make this work. This strategy could be applied to the startup interp as well but that would mean never eliminating the AST in any case where it coudl fall back. Memory tradeoff concern there. |
@danmayer FWIW I just landed #6180 on master, which does two things:
The implementation largely follows what I suggested in #3096 (comment), which means the eventual JIT code will either have no coverage overhead at all, or it will use invokedynamic to limit that coverage to the first time executing a line. The native code that results should never have coverage overhead since it will always have run at least once before the native JIT gets to it. There are some caveats though:
|
wow, awesome to see/hear @headius yeah Ruby coverage has some issue with line number on multi-line statements. No worries on |
I'm going to close this issue. The Coverage discussion was only tangentially related because it was based on event hooks before. Now it updates coverage data directly from the LineNumber instructions, and it can be used at any time without enabling debug mode. So that leaves We can make the event itself be basically free and always-on using the same trick as for In addition, even though we could make the events be always-on, we can't do the same for the binding.
For Since it seems like most of the interest is in always-on Coverage rather than tracing, and we've implemented that in #6180, we'll call the rest of this "won't fix" until we can be convinced that the extra code size and warmup hit are worth always-on tracing. |
@danmayer We would love to have you try out the improved Coverage support on JRuby master! There are snapshot builds for every green CI run here: https://oss.sonatype.org/content/repositories/snapshots/org/jruby/jruby-dist/9.3.0.0-SNAPSHOT/ And every night the most recent of these is repackaged as a nightly build here: https://github.com/ruby/jruby-dev-builder/releases/latest We do intend to match CRuby's line numbers so that will get fixed... but perhaps this works well enough for CoverBand already? If you see issues that are not due to multi-line statements' line numbers, please open a new bug. |
I will try to give this a go this weekend thanks |
I got this installed and was able to verify basic scripts and get most of Coverband passing. I did get one error:
This is defined here in CRuby: https://github.com/ruby/ruby/blob/c5eb24349a4535948514fe765c3ddb0628d81004/ext/coverage/lib/coverage.rb Given it is just Ruby, I can work around this, but just adding that extension when running JRuby
but I wanted to let you know, as I am not sure if that is something you would want to just directly support @headius |
saw that you. wanted anything else in a new issue, so I created #6181 |
@danmayer Thank you! |
After loading up my application using 9k, it prints following warning all over the place (and which breaks my application code, if I dont pass debug):
This behavior is divergent from MRI's behavior, which as I understand you try to mirror as close as possible, so I'm wondering whether this diversion is intentional or not. If it is, I'd like to appeal that it be removed.
Truthfully, the real reason I'd like for the restriction to be removed has nothing to do with above but rather, I have numerous libraries where I've been (ab?)using TracePoint (which uses set_trace_func), with regardless of whether you prefix the word or not, has been to much success.
The best use case (and an example of something that cant be done in ruby without set_trace_func), can be seen here (an abstract methods implementation). Because when the module is included I cant check which methods have been defined on the class, as module included is called before the method definitions, I can get around this by watching for the end of the class definition using trace point. I can also use a simple class instance variable setter pattern to store the method names, since all of that can happen, before calling the after_included hook allowed by tracepoint.
https://github.com/jasonayre/trax_core/blob/master/lib/trax/core/abstract_methods.rb
Because set_trace_func fires off a class end event for the current class being watched, I am able to check for the the end of the class definition, and determine whether a method was implemented, and throw an error if not.
After using TracePoint class to solve the abstract method problem, it opened my eyes to using it in several other areas, to solve various problems which are much easier to solve with deferred class evaluation (i.e. a mixin pattern which allows you to pass arguments into the module, or set them using class instance variables, which once again allows you to run the configuration behavior of the module being included lazily.)
It may strike you as janky using a library intended for debugging to do these kind of things, but I've compared for example the mixin implementations side by side, and the TracePoint version makes things much simpler, as well as opens up quite a few possibilities.
So anyways, not sure if this was even intentional since this diverges from MRI, but in the event it's not, I figured I'd plead my case for the reversion of said behavior :)
The text was updated successfully, but these errors were encountered: