Ruby 2.7: Refinements take place at Object#method and Module#instance_method #2177
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eregon
reviewed
Dec 7, 2020
| @@ -1352,6 +1354,15 @@ protected RubyMethod method(VirtualFrame frame, Object self, Object name, | |||
| @Cached ConditionProfile notFoundProfile, | |||
| @Cached ConditionProfile respondToMissingProfile, | |||
| @Cached LogicalClassNode logicalClassNode) { | |||
| final Frame callerFrame = getContext() | |||
| .getCallStack() | |||
| .getCallerFrameIgnoringSend(FrameInstance.FrameAccess.READ_ONLY); | |||
There was a problem hiding this comment.
Kernel#method should be fast and basically just be an inline cache on the instance class + name.
This causes to iterate the stack every time.
Let's use the same approach as in RespondToNode.
There was a problem hiding this comment.
Hm, but ReadCallerFrameNode (used in RespondToNode) also iterate the stack, isn't?
It just has a condition profiler to "skip" an iteration sometimes (not sure how often)
There was a problem hiding this comment.
The first branch is basically always taken after the first call for a given call site, so it only iterates once per call site, and then all later calls just read the caller frame from the arguments of the current frame (except edge cases like a non-Ruby caller, but we can detect those by being passed a null frame or so).
|
Here is a quick summary of the work & progress of this issue. The whole idea of supporting refinements for
We need to tweek
Then we had a conversation in Slack about the solution and the final thought is:
As a result, I would be happy to get a review of the last solution and a hint of what I am doing wrong 🙏 |
|
My apologies for the super late reply. The error is from Regarding type casting, I think we can exploit the fact that MaterializedFrame subclasses VirtualFrame, but we might not even need it. |
eregon
force-pushed
the
feat/2.7-refinements-for-method
branch
from
286ad8d
to
237d2a5
Compare
ssnickolay
force-pushed
the
feat/2.7-refinements-for-method
branch
3 times, most recently
from
8aea66c
to
783c3cc
Compare
|
@eregon we are ready for another review-round 🧑🍳 |
eregon
reviewed
Jan 25, 2021
src/main/java/org/truffleruby/language/methods/GetMethodObjectNode.java
Outdated
Show resolved
Hide resolved
src/main/java/org/truffleruby/language/methods/LookupMethodOnSelfNode.java
Outdated
Show resolved
Hide resolved
ssnickolay
force-pushed
the
feat/2.7-refinements-for-method
branch
from
849d51a
to
01ef61a
Compare
ssnickolay
force-pushed
the
feat/2.7-refinements-for-method
branch
from
01ef61a
to
2d95cfe
Compare
eregon
approved these changes
Jan 29, 2021
* Now that we found out that having a public execute(Frame, ...) is fine.
eregon
reviewed
Jan 29, 2021
| } | ||
| final RubyMethod instance = new RubyMethod( | ||
| context.getCoreLibrary().methodClass, | ||
| RubyLanguage.getCurrentLanguage().methodShape, |
There was a problem hiding this comment.
RubyLanguage.getCurrentLanguage() is slow, it's essentially a ThreadLocal lookup.
So I switched to @CachedLanguage instead.
…sed a reason * neverPartOfCompilation() is much safer to use now that it is checked statically when building a Native Image.
…erPartOfCompilation * neverPartOfCompilation() is much safer to use now that it is checked statically when building a Native Image.
|
I added a check that RubyLanguage.getCurrentLanguage/getCurrentContext are never used in PE code, so the gate would nicely catch issues like #2177 (comment). |
|
It looks like TCK tests fail, I'll take a look: |
…mbol name from Kernel#method
…_to_missing? and #method_missing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #2004
A few notes:
#methodis similar to#respond_to?(see: https://github.com/oracle/truffleruby/pull/2120/files) where we usedReadCallerFrameNode. In this PR I don't useReadCallerFrameNode, but instead directly call:to avoid frame materialization. However, we discussed the possible optimization of
ReadCallerFrameNodeso maybe we should repeat theRespondToNodeapproach (with@Child private ReadCallerFrameNode readCallerFrame) to be more consistent.I didn't find
#methodor#instance_methoduses via Primitive (so there are no needs in : d84da65).There were also options with AST-inlining, but we haven't done that yet.
How do you think this is expected?