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

Split frame fields to reduce pre/post logic #4905

Merged
merged 12 commits into from Dec 22, 2017

Conversation

Projects
None yet
3 participants
@headius
Member

headius commented Dec 19, 2017

One of the primary perf hits in JRuby comes from methods that need a method frame to store data for downstream calls. An example: because there's one [] method in the system that needs to write the "backref" frame field, any method that calls [] on any type needs to have "backref" storage available.

Unfortunately, there's problems with how we manage this now:

  • The need for any frame field force a full frame, with multiple writes in pre and post logic to set up and tear down that frame. If only one field is used, this is wasteful.
  • There's no way to tell, at optimization time, that only one frame field is needed.

This PR starts the process of splitting frame push/pop logic to be more fine-grained, allowing for initializing only the parts of the frame we need.

At the moment, this PR does the following:

  • Gathers more detailed information on what frame reads and writes might be associated with a given method name.
  • Pushes this information through into the scope flags via the current flag calculation mechanism.
  • Uses the reads and writes to know whether this scope needs a full frame or just a "backref frame" that only manipulates one field.

In experiments, for a method that contains a call to [] but otherwise does nothing, 30 million calls (in JIT) improve from 0.33s down to 0.08s or so, around 4x faster by removing unnecessary frame overhead.

headius added some commits Dec 15, 2017

@headius headius added this to the JRuby 9.2.0.0 milestone Dec 19, 2017

@headius headius requested review from enebo and subbuss Dec 19, 2017

@enebo

enebo approved these changes Dec 19, 2017

Already addressed my concerns offline. This is good stuff.

@subbuss

This comment has been minimized.

Contributor

subbuss commented Dec 19, 2017

So, the basic premise here is that the only source of use of some of the frame fields comes from the ruby stdlib, and for the JRuby impls in Java, those can be annotated, and optimized around.

Looks like this PR is asserting that this claim is true for the backref field.

Are there any frame fields where that premise doesn't hold?

@subbuss

This comment has been minimized.

Contributor

subbuss commented Dec 19, 2017

i.e. can non-stdlib ruby methods directly (vs indirectly via use of stdlib methods) affect use of any of these frame fields?

@headius

This comment has been minimized.

Member

headius commented Dec 19, 2017

@subbuss Non-stdlib methods could only affect this if they were also defined in Java and annotated to access these fields.

We do update the list of methods that might need frame fields as they are defined, so if a third-party lib has a JRuby extension and some methods that need frame, the list of "might need a frame" method names will get updated. However this is obviously timing-sensitive.

I chatted with @enebo about ways to mitigate this risk.

The main potential problem with this PR is the following scenario:

  • A method contains calls that are known to need backref.
  • The method also contains calls that need other frame fields, but for whatever reason we can't see that (aliasing, for example).
  • Now we will only set up space for backref.
  • Additional frame use will not be initialized or cleared properly.

This requires a lot of things to align. The most likely way to have a method call that needs framing but doesn't get it is to alias one of the core methods. This is done rarely, since aliasing is usually used to wrap a method, and you can't wrap methods that access the caller's frame. The other way to trigger this problem is as you mentioned, a third-party library that is using our annotations.

@subbuss

This comment has been minimized.

Contributor

subbuss commented Dec 19, 2017

@subbuss Non-stdlib methods could only affect this if they were also defined in Java and annotated to access these fields.

I see, so default in the absence of annotations is requiring the full frame?

That default question comes from: can a pure ruby method have code that can affect use of any frame field? I suppose that only applies to the backref field since that is the only one you opt right now.

@headius

This comment has been minimized.

Member

headius commented Dec 19, 2017

@subbuss It is not possible for pure Ruby code to access the caller's frame unless it is passed a binding or closure (which can be turned into a binding).

@headius

This comment has been minimized.

Member

headius commented Dec 19, 2017

To clarify...a method can certainly have its own calls to [] or binding or whatever, but that's different than needing access to the caller's frame, like several core methods do. It's not possible to access the caller's frame from Ruby unless it was handed to you.

@subbuss

This comment has been minimized.

Contributor

subbuss commented Dec 19, 2017

Ah, right. I forgot that detail .. been a while. :) Didn't look at the code closely, but the idea lgtm.

headius added some commits Dec 19, 2017

Allow aliasing to update the frame reads/writes for a name.
Also cleaned up concurrent access to these structures.

@headius headius merged commit 34aae7c into master Dec 22, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@headius headius deleted the split_frame branch Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment