Improve inlining of varargs path for split methods#6633
Merged
headius merged 4 commits intojruby:masterfrom Mar 26, 2021
Merged
Improve inlining of varargs path for split methods#6633headius merged 4 commits intojruby:masterfrom
headius merged 4 commits intojruby:masterfrom
Conversation
Member
Author
|
This improves the performance of the But I am seeing some peculiar errors locally I will need to investigate before this is merged: |
headius
force-pushed
the
split_varargs_indy_better
branch
from
e19b533 to
0ceb25e
Compare
Member
Author
|
Silly bug in my original code. Worrisome that it was not picked up by testing. 🤔 This is fine now. |
headius
force-pushed
the
split_varargs_indy_better
branch
2 times, most recently
from
2e87c61 to
557e713
Compare
Member
Author
|
Rebasing on master to reduce risk for 9.2.x. |
When we generate the invoker class for arity-split methods, we only generate overrides of (DynamicMethod subtype) JavaMethod.call signatures that align with the split signatures, including whether or not they receive a block. This allows the base JavaMethod to maintain a minimal set of base implementations, with unimplemented arities and block paths falling back on default implementations. Unfortunately due to our indy dispatch logic only being smart enough to directly bind the lowest specific arity, this can break inlining for all other arities if the block/no-block signatures do not match, falling back on the base implementations in JavaMethod. This patch improves the "generic" DynamicMethod.call binding to avoid passing a block when the target methods is backed by a direct native handle and that handle wants a block. Because we only allow homogeneous block/no-block signatures for arity-split methods, omitting the block allows inlining straight through the JavaMethod invoker even when calling the other arities.
I will do some separate work to reduce or eliminate null NativeCall from methods that implement NativeCallMethod.
Since compiled Ruby bodies always receive block, this will mostly help native method_missing impls that do not receive block (for example, our Java package modules.
headius
force-pushed
the
split_varargs_indy_better
branch
from
588efc9 to
13153a9
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
When we generate the invoker class for arity-split methods, we
only generate overrides of (DynamicMethod subtype) JavaMethod.call
signatures that align with the split signatures, including whether
or not they receive a block. This allows the base JavaMethod to
maintain a minimal set of base implementations, with unimplemented
arities and block paths falling back on default implementations.
Unfortunately due to our indy dispatch logic only being smart
enough to directly bind the lowest specific arity, this can break
inlining for all other arities if the block/no-block signatures
do not match, falling back on the base implementations in
JavaMethod.
This patch improves the "generic" DynamicMethod.call binding to
avoid passing a block when the target methods is backed by a
direct native handle and that handle wants a block. Because we
only allow homogeneous block/no-block signatures for arity-split
methods, omitting the block allows inlining straight through the
JavaMethod invoker even when calling the other arities.