Skip to content
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

Rework method invocation to properly super #5627

Merged
merged 46 commits into from Feb 26, 2019
Merged

Rework method invocation to properly super #5627

merged 46 commits into from Feb 26, 2019

Conversation

@headius
Copy link
Member

@headius headius commented Feb 25, 2019

This PR reworks most of our method invocation pipeline to follow MRI's way to set up the frame class.

In MRI, when searching for a method in a class hierarchy, the method is returned along with the class in the hierarchy that held it. In the case of modules, this would be the included module wrapper inserted into the class hierarchy. By returning the wrapper, the "frame class" points at the proper place from which to start "supering" to the next level in the hierarchy.

JRuby's logic previously had to perform a search against the hierarchy every time a module method performed super, since we set up the frame class pointing at the module itself.

With the new logic, the CacheEntry returned from RubyModule.searchWithCache contains an additional reference to the appropriate class in the hierarchy from which to super. This eliminates the hierarchy search in all but a few places and should fix other bugs with double included modules, refinements, and prepends/includes.

headius added 30 commits Feb 5, 2019
Directly creating CacheEntry skips specialized cache entry
factories like synchronized or profiling.
jruby-openssl pregenerates its invokers, so we need to continue
to have the old signatures until it updates.
headius added 2 commits Feb 26, 2019
Include searches the entire hierarchy for a dupe, while prepend
only searches up to the next real parent class. This corresponds
to TRUE and FALSE values passed to MRI's include_modules_at for
search_super.
Copy link
Member

@olleolleolle olleolleolle left a comment

Found a tiny typo.

@kares
kares approved these changes Feb 26, 2019
Copy link
Member

@kares kares left a comment

nice work - was looking into changes from branch even before its was ready
CacheEntry seems to get a lot of attention here, it does not leak much into public API ✔️

@enebo
enebo approved these changes Feb 26, 2019
Copy link
Member

@enebo enebo left a comment

I have one comment which I almost marked request changes but since it has already been inconsistent (searchMethodCommon) I will merely suggest we match those two impls to return the same tuple of things.

We talked yesterday about the potential future confusion of searchMethod versus getting the CacheEntry back. We definitely should make a blessed API for future uses of people who want to lookup and call ala callsite caching (which is probably the lambda + indy soln you mentioned). I would say we should deprecate searchMethod (although it is such a nice name). Maybe we make a new named version of searchMethod (no idea on that new name), mark it protected, use it internally, and deprecate searchMethod? I think the name is so screaming use me we should do something.

Co-Authored-By: headius <headius@headius.com>
@headius headius merged commit c6121dc into master Feb 26, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@headius headius deleted the refinements2 branch Feb 26, 2019
@headius headius added this to the JRuby 9.2.7.0 milestone Mar 13, 2019
headius added a commit that referenced this pull request Mar 13, 2019
Fixes in #5627 appear to have made this unnecessary. The remaining
changes to support super from our native cgi/escape are still
necessary, however.

Fixes #4531.
eregon added a commit to ruby/spec that referenced this pull request Mar 27, 2019
eregon added a commit to ruby/spec that referenced this pull request Mar 27, 2019
Exposed a bug in JRuby super logic refit. See jruby/jruby#5627.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants