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

Test indy invokers #3593

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Test indy invokers #3593

wants to merge 8 commits into from

Conversation

headius
Copy link
Member

@headius headius commented Jan 12, 2016

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.

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.
@headius
Copy link
Member Author

headius commented Jan 12, 2016

I forgot to mention this change also results in a 1.6MB reduction in the size of jruby.jar.

@eregon
Copy link
Member

eregon commented Jul 26, 2016

👍 on anything simplifying the build :)

@kares
Copy link
Member

kares commented May 20, 2018

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants