Improve handling of chaining #require_relative patch #401
Conversation
ghost
added
the
| location = caller_locations(1..1).first | ||
| location = caller_locations.find { |x| x.base_label != "require_relative" } |
There was a problem hiding this comment.
Hum this increase the cost quite significantly.
Out of curiosity what else decorates require_relative? Maybe we could do something like caller_locations(1..3) to support up to two extra decorators?
There was a problem hiding this comment.
Honestly, not aware of any public Gem decorate it, but our projects internally is about to decorate it to track some state change pre/post file load, and it's blocked by the decorator in bootnap.
Only traverse the first few caller seems fine to me. The chaining of require_relative may not happen that often probably.
There was a problem hiding this comment.
I'll have to look on Monday if we even still need to decorate require_relative. I think it we might be able to remove this decorator entirely.
There was a problem hiding this comment.
Ok, so this is the change I remembered that may allow us to remove the require_relative decorator: ruby/ruby@79a4484. It shipped with Ruby 3.1, so that wouldn't remove it for everyone, but I'm much less concerned about a small perf impact if it doesn't apply to the newest rubies.
The decorator was added in #136, I'll have to dig more to see what it would take to remove it on 3.1+.
There was a problem hiding this comment.
Hum, the more I dig into #136, the more it seems like we tried to limit a Ruby bug more than a Bootsnap bug.
Assuming a/test.rb and b/test.rb symlink each others, the following load the file twice on all rubies but 3.1:
require_relative "b/test.rb"
require_relative "a/test.rb"So I don't think we should have done this at all.
|
So after digging into it for most of the day, I confirm that we can simply get rid of the |
|
That would work. I am closing the PR then. Thanks for the investigation, folks! |
|
Welcome. I might not release this right away as there's a couple other things I'd like to include. I recommend using the main branch in your Gemfile if you need a fix right away. |
Kernel.require_relative could be patched again by other Gems initialized after bootsnap. This change supports the chaining of Kernel.require_relative