-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Test indy invokers #3593
Open
headius
wants to merge
8
commits into
master
Choose a base branch
from
test-indy-invokers
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Test indy invokers #3593
+1,439
−553
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
MethodDescriptor is the common base class and calculates most fields based on overridden functions in the specialized types. ExecutableElementDescriptor is the version used at compile time against the javac model. JavaMethodDescriptor is the version used at runtime against reflected method objects.
HandleMethod has existed for a while, allowing us to have Ruby core methods implemented in Java bound without using bytecode- generated invoker classes. However the cost of building all the method handles necessary makes the indy approach slower than the generated code. Part of this is due to the reflective walking of all classes done at every boot to bind their methods. This commit fixes that by adding a new offline (annotation-processing) code generator that spins a new version of our "populator" classes for indy binding. These classes just contain method construction and binding calls, avoiding reflective class and method walking. They also contain all direct method handles for the core methods as constant pool entries, avoiding the checks required to acquire those at runtime. This was not enough to beat the generated invokers for boot time, because the direct handles still needed to be adapted for different arities, heap framing, and argument casting and reordering. In order to reduce those costs, HandleMethod will now defer that adaptation on a per-arity basis until the adapted handle is actually needed. This reduces boot costs of the handle- based methods to equivalent or less than the generated invokers. Caveats: * This is not yet passing all tests. In particular there's some bit-rotted indy binding code for HandleMethod that appears to need some work. * Boot time was tested with "-e 1" (about the same speed) and "gem list" (a bit faster with handles). Other cases may vary. * For an application that forces many handles to get adapted, this could lead to slower boot time. However, most applications only ever call a fraction of the methods defined in the system. * There appear to be significant memory improvements when running in this mode. Base memory for a "-e sleep" process dropped from 125MB to 95MB and the number of loaded classes dropped from 6600 to around 4400. Heap utilization was slightly lower at 11MB versus 12-15MB. I will push a separate PR that enables fully handle-based mode on a separate branch for experimentation, discussion, and fixes.
I forgot to mention this change also results in a 1.6MB reduction in the size of jruby.jar. |
👍 on anything simplifying the build :) |
would be interesting to have this in at some 9.2 -> maybe starting as an optional feature with a --switch |
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.
This change enables MethodHandle-based invokers for all native method binding, eliminating our pregenerated INVOKER classes and avoiding code generation at runtime for third-party extensions.
For my quick tests, it boots in about the same time or less time than the generated invokers and existing AnnotationBinder-created populators, but I'm also deferring a substantial amount of work until methods are first called. This could be smart, or it could be just delaying the inevitable...but so far it seems to balance out well.
See commit fc4bcb5 for a more detailed discussion of the changes. This PR should not be merged until it is passing tests and we have decided to commit to this direction.