-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Could we request a new Ruby API to remove the need for TracePoint? #135
Comments
Hey @casperisfine! Before fully committing to work on the library, I wanted to validate the impact of the TracePoint technique. There were some benchmarks performed, and Discourse ran with it, and the result was that it did not seem to be noticeable. That was key to move on with the project. I dislike usage of TracePoint because I do not find it elegant, so I am definitely in favor of a better API, but that is a different motivation and my idea was that if with time new APIs appeared, I would adopt them, but would not push for them myself. However, your motivation is performance and would like to understand the cost with actual numbers. Could you share some please making sure the benchmark uses TracePoint like Zeitwerk does? If motivation is performance, would we incur in the risk of asking for an API like |
Sure. I'll try to get you some numbers next week.
Actually, just compiling iseq with and without |
Hum, actually it might not be so obvious anymore, since 2.5 the |
Awesome Jean. Let's keep this open then by now 👍. |
So @XrXr did some benchmarking using opt-carrot: XrXr/optcarrot@d2b7ad1
So according to this benchmark, enabling a TracePoint once, even if it is then immediately disabled has a |
@XrXr To make sure we compare apples to apples, would you mind repeating the benchmark for a TracePoint that listens to the Let's disable using the same interface as well for the same price (guess it's equivalent, but still): TracePoint.new(:class){}.disable (For reference, TracePoint usage is all within this file.) |
Seem like you had some flair here. I did repeat Alan's benchmark difference with
So without looking at the code, it seems that that tracepoint impact is limited to the events that were listened to. So we can assume that the slowdown is limited to class/module creations (things opt-carot don't do), which should be fairly rare in production past boot time. |
🙌 |
So apparently TracePoint is causing issues to MJIT: https://k0kubun.medium.com/ruby-3-jit-can-make-rails-faster-756310f235a, but doesn't mean Zeitwerk needs to change, apparently Also if I understand https://bugs.ruby-lang.org/issues/14104 correctly, the cost now mist mostly when calling |
Yeah, we talked about this today on Twitter (don't know if you can see the conversation without being logged in). |
Ah nice. Yes, it's public. |
By now, I'd lean on waiting a bit and see, because the JIT is still a work in progress and @k0kubun is going to study options. I also offered to adjust Zeitwerk as needed if that could make his work easier, like |
Yeah, I also asked Alan, and according to him YJIT doesn't really have this problem. Another argument for pushing for another API would be that the usage of TracePoint breaks But even with all this, I still feel uneasy with using TracePoint. If I get some time to hack on Ruby I might try my hand at the "nested |
If you do that I go to France and invite you to dinner. I've always seen TracePoint usage in Zeitwerk as a necessary evil I'd love to avoid. I ensured even before writing code that performance was not affected, but from an implementation standpoint, it feels a bit off to me. Only, I do not know any other technique today to support explicit namespaces. Support for nested autoloads would be super. Off the top of my head, I think we could still be lazy visiting the tree by using that feature only in the case of explicit namespaces. A callback on class/module definition could also make it, if invoked before the class body is executed. |
Do you have a preference for either? I might just go with the simplest, but still good to know your opinion. |
Intuitively, the callback. It would be simpler to implement, it is all Zeitwerk needs, there already exist callbacks for other things in Ruby, and Zeitwerk would preserve its controlled/uniform descend. |
So turns out the implementation is quite trivial: ruby/ruby#4521 |
Also feel free to chime in https://bugs.ruby-lang.org/issues/17881 |
👋 @fxn, First, I know you like to keep the issues to 0, so I'm very sorry for opening such an open ended one. We can close it immediately if you wish 😉 .
Context
Zeitwerk need some hook to know when a new module or class is defined, so that it can define the required
autoloads
on it.The problem I have with TracePoint is that it has a fairly significant cost, even when disabled. The need for a workable TracePoint means we can't compile iseqs with tracing disabled (
RubyVM::InstructionSequence.compile_option = { trace_instruction: false }
), and that the VM will be constantly checking wether a tracepoint is enabled. Even if none are, it causes some overhead. So medium/long term I'd like to eliminate this need.So I'm thinking, is there some better, more specific API we could request to ruby-core, so that hopefully in Ruby 2.8/3 or later, Zeitwerk could no longer depend on
TracePoint
?Ideas
const_added
Maybe Ruby could have a
const_added
callback similar to themethod_added
one? I think it could work, Zeitwerk would define one inKernel/Object
, however if a class/module was to redefine it without callingsuper
, it would break autoloading. It might also be tricky to make it work withBasicObject
.However it seems relatively straight forward to implement.
nested
autoload
Another nice API could be to allow nested const names in autoload, e.g.
autoload "Foo::Bar", some_path
. Unless I'm missing something, this would dramatically simplify Zeitwerk, you could just compute a list of all loadable constants, and directly register them all.This might be slightly harder to implement, but seems ideal.
something else?
Maybe there are other APIs you had in mind while implementing Zeitwerk?
The text was updated successfully, but these errors were encountered: