New approach to finding attached class

[egiurleo]

Closes #1097.

Motivation

Historically, to identify the attached class of a singleton class, we've parsed the name (result of the#inspect method) of the singleton class and then constantized the result. However, this doesn't work when the #inspect method of a singleton class is overridden because the return value of #inspect no longer conforms to the structure we expected.

Implementation

We can fix this problem by searching ObjectSpace for the class that has the correct singleton class. This is less performant but more correct.

Tests

Added a test that fails without the changes and passes with them. Existing tests continue to pass.

[paracycle]

Just a small comment, otherwise this looks good, thanks!

[paracycle]

Previously we were skipping the rest of the processing if the attached class wasn't found (if the name wasn't available). In the new version we continue processing if attached_class is nil, which might lead to changes in behaviour in the future if more code is added after this block.

[egiurleo]

Oh good catch. Thanks for pointing that out.

[rafaelfranca]

What is the performance of this in a big app like Shopify/shopify? There are a lot of objects in the object space in an application of that size, so looping though it doesn't seems to be as fast as the previous implementation.

[vinistock]

Question: would an approach similar to this one work? I mean invoking the original inspect and then keeping the original implementation. And if not, what are the differences?

[paracycle]

Question: would an approach similar to this one work? I mean invoking the original inspect and then keeping the original implementation. And if not, what are the differences?

This was discussed in Slack, unfortunately that trick doesn't work, since we would be calling inspect on the singleton class, but that internally does an inspect call on the attached class to get its name, so that it can build the string #<Class:AttachedClassName>. Since we have no control over the internal call to inspect on the attached class, we can't avoid the problem, it still ends up calling the user defined method.

[egiurleo]

What is the performance of this in a big app like Shopify/shopify? There are a lot of objects in the object space in an application of that size, so looping though it doesn't seems to be as fast as the previous implementation.

Good question! Because this is run during the tapioca gem command, the only objects that should be iterated in the object space are the ones loaded by the gem, not by the application. So this code should never be run while Shopify/shopify is loaded.

Also, this code is only run when monkeypatching a singleton class defined in another gem (e.g. if you were to call ActiveRecord::SchemaMigration.singleton_class.include(Foo) in the foo gem), which I'm assuming most gems don't do at all and some gems do sparingly.

This is definitely a less performant implementation but it should never iterate through an object space as large as Shopify's and should only be run on a very specific edge case.

[amomchilov]

Would it be useful to maintain a cache of these, so we can at least cap the look-ups to once per singleton_class?

[amomchilov]

Also, could we add a fast path?

If inspect is not overridden, then we can use the old technique

[egiurleo]

@amomchilov both interesting thoughts!

Re: caching, I wonder if it would really be worth the amount of memory used, considering it's probably pretty rare for the same singleton class to have multiple dynamic mixins added to it. How would you think of the tradeoffs there?

Re: fast path, I really like that! What do you think of something like this?

def attached_class_of(singleton_class)
  if singleton_class.method(:inspect).owner == Module
    # old way here
  else
    # new way here
  end
end

cc @paracycle

[amomchilov]

How would you think of the trade-offs there?

No clue, I'm just thinking out loud :)

What do you think of something like this?

Yep, that's pretty much what I had in mind!

Worth noting:

• caching and fast-pathing are orthogonal ideas.
• If the fast path is common enough, a catch-all cache would be less useful
• We could add caching to only the slow path.
  • It would be smaller (few classes hit the slow path)
  • It would only be applied where it would actually help (i.e. not on the fast path)
  • OFC it's still only worthwhile if we find we're hitting attached_class_of for the same class multiple times. Needs research.

[paracycle]

I don't think that pseudocode would work since the problem is that the inspect method is overriden not on the singleton class but on the original class, i.e. #<Class:Foo> has the default inspect, but Foo has an overriden inspect method. Since we don't have a handle on Foo to begin with, we can't check if its inspect method is overriden either.

[paracycle]

The optimization we can do here is to do the object space walk just once over all Module descendants and create a lookup table from each singleton class to its attached class in that whole hierarchy. After that, each call to this method would be an O(1) operation (a simple lookup).

However, it would be critical that we do that after all of the classes that expect to be loaded are actually loaded and any mistake there might be costly. I'd rather we keep the less performant version for now and optimize later if we see it being a problem.

[amomchilov]

Re: comment 1: hahaha good point.

Re: any mistake there might be costly. I'd rather we keep the less performant version for now and optimize later if we see it being a problem.

Agree.

[michaelherold]

I had a thought after reading the conversation in the other thread. Would it perhaps make sense to do something like:

MODULE_INSPECT_METHOD = T.let(Module.method(:inspect).unbind, UnboundMethod)

name = MODULE_INSPECT_METHOD.bind_call(constant)

Since the problem is that inspect may be overridden, what if we ... just didn't use it?

The only case where I could see this being an issue is for anonymous classes, but that behavior wouldn't apply to foreign constants in the first place.

[amomchilov]

Another remark:

Looks like singleton classes already have a pointer to their attached classes in underlying implementation (at least in MRI). Which is like, obvious when I think about it (d'uh, otherwise how could they generate the ::inspect?).

It's currently stored in an ivar called id__attached (the constant in the C code is id__attached__):

https://github.com/ruby/ruby/blob/4325e90205aa4cd0ea031df1b5e6334bfd9c7e51/class.c#L33

Usage:

https://github.com/ruby/ruby/blob/ff07e5c264c82f73b0368dd0bc2ae39f78678519/object.c#L1542

Perhaps we could pitch making a public API that exposes this, instead of using this Stringy hack to begin with? (long term, this PR can still proceed as normal)

[paracycle]

@michaelherold That won't work for the reason that @amomchilov describes above and I describe here.

@amomchilov That already exists, and I'd chimed in on that thread too: https://bugs.ruby-lang.org/issues/12084

[amomchilov]

@paracycle attached_object makes perfect sense to me. Perhaps we can link this pr in a comment on that thread as a concrete use case

[paracycle]

I can try to make another case on the issue with reflection/introspection as the main use case.

[paracycle]

I made a case for it here: https://bugs.ruby-lang.org/issues/12084#note-11