Project

General

Profile

Actions

Feature #17881

open

Add a Module#const_added callback

Added by byroot (Jean Boussier) 5 months ago. Updated 2 months ago.

Status:
Feedback
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:103974]

Description

Use case

Autoloaders like zeitwerk need a callback when a new class or module is registered in the constant table.

Currently this is implemented with TracePoint's :class event. It works, but it is a bit unfortunate to have to use an API intended for debugging to implement production features. It doesn't feel "conceptually clean".

It also doesn't play well with MJIT, even though it's more of an MJIT limitation.

Additionally this usage of TracePoint cause some incompatibilities with some debuggers like byebug (even though others don't have this issue).

Proposal

I believe that if Ruby was to call Module#const_added when a constant is registered, Zeitwerk could get rid of TracePoint.

For now I implemented it as: const_added(const_name) for similarity with method_added. But maybe it could make sense to have the signature be const_added(const_name, const_value).

Also since method_removed exists, maybe const_removed would need to be added for consistency.

Links

Patch: https://github.com/ruby/ruby/pull/4521
Zeitwerk side discussion: https://github.com/fxn/zeitwerk/issues/135

cc k0kubun (Takashi Kokubun)

Updated by Eregon (Benoit Daloze) 5 months ago

This would trigger for all constants, which might affect startup quite a bit by having all those extra calls (I already see it as a problem for method_added, but maybe we can optimize for the common case that it's not redefined).
Maybe we should have a callback just for modules/classes bodies, exactly like the :class TracePoint?

I think a JIT doesn't need to give up anything for the :class TracePoint.
In fact, it even seems reasonable to always check if there is an active :class TracePoint when entering a module body, module bodies are after all rarely executed more than once, and the check should be fairly cheap (especially compared to creating a new Module).

Updated by byroot (Jean Boussier) 5 months ago

This would trigger for all constants

Indeed, as it seemed the most consistent API to me. But I'm open to an alternative that would only trigger for class / module. Not sure how it could be named though.

which might affect startup quite a bit by having all those extra calls

I haven't measured, but I doubt it would be slower than the current TracePoint.new(:class) callback.

I already see it as a problem for method_added

We have that hook defined in our app because of sorbet, and the overhead really isn't that bad.

I think a JIT doesn't need to give up anything for the :class TracePoint.

Of course, it's only a current MJIT limitation. I only mentioned it because k0kubun (Takashi Kokubun)'s blog post is what prompted me to look at this again. So it's definitely not the main reason to do this, it could just happen to be a nice side effect. A JIT that strip tracing code, and simply de-optimize when tracing is enabled doesn't seem that far fetched to me.

Actions #3

Updated by byroot (Jean Boussier) 5 months ago

  • Description updated (diff)

Updated by Eregon (Benoit Daloze) 5 months ago

  • Description updated (diff)

A JIT that strip tracing code, and simply de-optimize when tracing is enabled doesn't seem that far fetched to me.

Yes, and I think it makes a lot of sense to deoptimize/recompile for e.g., :line and :call events (those have a huge perf impact).
But for :class I don't see how avoiding the check at the cost of deoptimization/recompilation would ever be worth it.

Updated by Eregon (Benoit Daloze) 5 months ago

Ugh, Redmine doesn't handle posting a comment concurrently and now incorrectly reverted the description changes even though I didn't touch the description.
byroot (Jean Boussier) Could you re-apply your changes?

Actions #6

Updated by byroot (Jean Boussier) 5 months ago

  • Description updated (diff)

Updated by Eregon (Benoit Daloze) 5 months ago

One potential overhead of the :class TracePoint is that it allows accessing the binding through TracePoint#binding:

$ ruby -e 'TracePoint.new(:class) { |tp| p tp; p tp.self; p tp.binding.local_variables }.enable; a=1; class C; b=2; end' 
#<TracePoint:class@-e:1>
C
[:b]

I'm not sure how much it matters in practice, probably not a lot.

If we add some more direct callback instead we won't have that issue, but we'd need to lookup the callback method every time vs just checking a global flag "any class TracePoint enabled".

Updated by byroot (Jean Boussier) 5 months ago

Honestly I shouldn't have mentioned current performance at all in the first place, it's really not the main reason we'd like this new API.

However you do have a point that this new API should be at least as performant, otherwise it will be a difficult value proposition.

Updated by fxn (Xavier Noria) 5 months ago

Thanks a lot for working on this Jean.

If we add this, Zeitwerk would be using a somewhat more "normal" API. TracePoint is correct, but seems a bit off to me conceptually (I do not know if I should revise that perception, maybe should I?)

However, the comments by Eregon are key. The JIT is WIP and it is going to be revised, there are JITs that work with TP just fine, current Ruby has no issue with a TP on the :class event, etc. It would be a patch in light of proposing an API that makes sense and would have at least this use case, and would make Zeitwerk a tad less hacky (again, modulus technically it is correct).

There's also something that makes me think. The constants API, in general, does not distinguish constants defined for real, from constants that have only an autoload configured that has not been triggered. For example, if you autoload :Foo, "file", and "file" is not evaluated, the constants API for Object tells you Foo is a constant in Object.

So, to be coherent with this, perhaps const_added should be triggered in autoload calls. But that would not help Zeitwerk, because what we need is to grab the class/module object at the top of its class/module body. And, we do not need to be called on reopenings (something the :class event does).

In 2018, I even played with the idea of decorating some low-level method in Module that allowed me to intercept new class/module objects being created, but that does not seem reachable from Ruby, so took the TP as a necessary evil. It is the only technique I know to support explicit namespaces.

Updated by fxn (Xavier Noria) 5 months ago

Let me also add something for context. byroot (Jean Boussier) knows it and probably Eregon (Benoit Daloze) too, just for anybody else following.

Something the TP on the :class event allows you to do is to enable only if you need it. Allows you to be precise.

If the TP is not needed ever, it is not even enabled. If at some point you needed it, but no longer, Zeitwerk disables it. The tracer is managed by this module where you can see the fine logic in register/deregister.

A TP on the :class event is only needed if some of the loaders in the Ruby process saw both a file "foo.rb" and a directory "foo", and Foo has not been yet loaded. As soon as Foo is loaded, if there are no more constants in a similar situation, the tracer is disabled.

In particular, the tracer becomes normally disabled in a Rails application in production mode because Zeitwerk eager loads not only the application, but any other gem managed by Zeitwerk. That is what Zeitwerk::Loader.eager_load_all does here.

So, I find a callback to be neat from an implementation standpoint (if one associates TracePoint to debugging use cases), but you wouldn't normally be able to be so precise. Though you could probably at least bail out first thing in the block body via a maintained flag or something.

TP on the :class event, ironically, seems a tad off for me, but it happens to be perfect to solve this problem. There is no performance penalty that I know of, and in the case of JITs, on paper it should be fine (it is in a JIT byroot (Jean Boussier) checked).

So... no idea, it's core's call :).

Updated by byroot (Jean Boussier) 5 months ago

If the TP is not needed ever, it is not even enabled. If at some point you needed it, but no longer, Zeitwerk disables it.

True, but what worries me would be gems using Zeitwerk and not fully eagerloading, there's even an explicit API for doing just that highligthed in the readme: https://github.com/fxn/zeitwerk#eager-loading

db_adapters = "#{__dir__}/my_gem/db_adapters"
loader.do_not_eager_load(db_adapters)
loader.setup
loader.eager_load # won't eager load the database adapters

If I'm not mistaken, such usage could cause the TracePoint to stay active at runtime, which isn't a huge deal, but will still trigger every time a singleton_class or anonymous class is created, which isn't a good practice at runtime, but that I nonetheless see happen.

On the other hand, assigning constants is extremely rare past the boot phase, so I'm much less worried about it.

But again all this is hard to quantify and a bit "handwavy". I believe the cleanliness of the API alone should be the main argument, as well as not interfering with other TracePoint usages.

Updated by fxn (Xavier Noria) 5 months ago

If I'm not mistaken, such usage could cause the TracePoint to stay active at runtime, which isn't a huge deal, but will still trigger every time a singleton_class or anonymous class is created, which isn't a good practice at runtime, but that I nonetheless see happen.

Yes, that is right. Is in that sense that I said normally. You'd need someone to opt out from eager loading a subtree, and to have explicit namespaces in there in a level visited by the tree walker (which only descends on demand). It is a possibility, but I'd believe should be rare. Rare enough not to be your main design driver.

But again all this is hard to quantify and a bit "handwavy". I believe the cleanliness of the API alone should be the main argument

Agree, and I believe it is core's call to decide if that is cleaner, perhaps Zeitwerk's usage of TP is already "clean" in their opinion. I do not know :).

Updated by fxn (Xavier Noria) 5 months ago

Another gotcha I see is that we cannot assume const_added is exclusive to Zeitwerk. Users could have their own. So we would need to prepend our own, to make sure it is not rewritten. And on reload, we would need to keep track of which non-reloadable class/module already has the module prepended, not to prepend again. And would need to document that if you prepend, make sure to call super in case there's ours.

What I had in mind in the conversation in the Zeitwerk ticket was not const_added. It was a callback that gets called when a new class/module is created and after it gets its name set if defined with the class/module keywords. Analogous to what we use today. That is, some sort of global chainable callback "on_new_module(&block)", hence all these remarks :).

So, as I see it, these are some things to think about:

  1. If there's an autoload for Foo, Object.constants has Foo, Object.const_defined?(Foo) is true, etc. but you did not get a const_added call for it. That does not seem coherent. On the other hand, if we make it coherent, it is not usable by Zeitwerk. This is an important one, semantically.

  2. Zeitwerk cannot assume it controls const_added, needs cooperation from the user (this is doable via documentation).

  3. There is no performance issue to address that I know. I believe this patch should be discussed on a generic Ruby API context, independently of Zeitwerk.

  4. What would really work for Zeitwerk if we want a "cleaner" API is a chainable on_new_module(&block).

I am not exactly proposing on_new_module(&block) because the :class event of TP already does that (plus reopening, it is less precise), and because does no seem like existing Ruby hooks. I am mentioning this to highlight the difference between what I thought Zeitwerk could benefit from, from what we are discussing.

Updated by fxn (Xavier Noria) 5 months ago

Oh, and while Zeitwerk can disable the TracePoint, it wouldn't be able to disable on_new_module(&block), at most have a flag.

My personal take is: As far as Zeitwerk is concerned, I'd study some API if core believes we should not use a TP on the :class event.

Otherwise, personally, I would do nothing.

Then, the conversation is legit as Ruby API per se, if you like. But in that case, I believe you should get const_added called on autoload calls for consistency with existing semantics.

Updated by fxn (Xavier Noria) 5 months ago

If I'm not mistaken, such usage could cause the TracePoint to stay active at runtime, which isn't a huge deal, but will still trigger every time a singleton_class or anonymous class is created, which isn't a good practice at runtime, but that I nonetheless see happen.

For singleton classes, in the (rare?) eventual case that the tracer remains enabled we return immediately. The cost of that has to be absolutely negligible in practice.

Regarding anonymous classes and modules, remember TP does NOT trigger :class on Class.new or Module.new, which is perfect for our use case, since Zeitwerk is only interested in classes/modules with names.

Updated by byroot (Jean Boussier) 5 months ago

What I had in mind in the conversation in the Zeitwerk ticket was not const_added. It was a callback that gets called when a new class/module is created and after it gets its name set if defined with the class/module keywords.

I see, seems like I missed that.

I suppose the "ideal" API for an autoloader like Zeitwerk would be something like Module#module_defined(name, module):

  • Would trigger on classic module Foo::Bar first opening.
  • Would trigger on Foo::Bar = Class.new. (This one being a current limitation with TracePoint if I'm not mistaken).
  • Would not trigger on Foo.autoload(:Bar, "bar").
  • Would not trigger for non Module or Class constants.

Updated by fxn (Xavier Noria) 5 months ago

Would trigger on Foo::Bar = Class.new. (This one being a current limitation with TracePoint if I'm not mistaken)

Correct. Nowadays, you cannot define an explicit namespace with a constant assignment like that (docs), because Class.new does not trigger the :class event (test coverage, since a certain optimization is based on this).

Updated by fxn (Xavier Noria) 3 months ago

Followup here.

Before I started working on Zeitwerk, I benchmarked whether a TP enabled on the :class event had a measurable impact. Richard Schneeman and Sam Saffron ran benchmarks too. Tests said it did not, that validated the technique for explicit namespaces and gave me green light to go ahead.

k0kubun (Takashi Kokubun) recently ran a new synthetic benchmark with similar results and concluded:

... so, in summary, Zeitwerk doesn't have to change for VM, and thus we don't need new API for it. While both MJIT and YJIT have a challenge to optimize TP-enabled programs, :class events could be a special case which is easier to optimize at least than :line. I'll work on it.

I am told YJIT works fine with Zeitwerk (right byroot (Jean Boussier)?), and MJIT will work on it.

So, as far as Zeitwerk is concerned, I believe we are fine and we do not need new APIs.

This ticket could be pursued, if you like, for the sake of the Ruby language itself.

Updated by k0kubun (Takashi Kokubun) 2 months ago

  • Status changed from Open to Feedback

FYI, I changed the MJIT implementation in ac4d53bd461ff386cd45fdd484ffb6b628a251ad to not disable JIT-ed code when a TracePoint for class events gets enabled. So, as long as Zeitwerk only uses TracePoints for class events, we no longer need this API for MJIT w/ Zeitwerk either.

Actions

Also available in: Atom PDF