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

Improve refinements implementation #5604

Merged
merged 33 commits into from Feb 11, 2019

Conversation

Projects
None yet
2 participants
@headius
Copy link
Member

headius commented Feb 11, 2019

This is a reworking of the refinements logic in JRuby to use a mechanism similar to MRI, with marker entries in method tables to indicate that a given class + method has been refined somewhere.

There are a few pieces missing but this gets most refinement tests passing.

To do:

  • Optimization; very little caching is done along refined paths
  • Fix handling of super calls in modules. See #5585.

This branch is green but has a handful of specs/tests excluded due to the above. Branch refinements2 has ongoing work on the super issue.

headius added some commits Jan 7, 2019

Implement missing behavior for refinements based on MRI.
MRI's data structures for managing method tables are very similar
to ours, so I'm following their pattern of inserting wrapper or
dummy refined entries into classes that get refined elsewhere in
the system.

Here's a summary of the logic:

When defining a method:

* If the method does not already exist and the target module is a
  refinement module (isRefinement), add a refined method entry to
  the related class.
  * If the method also exists on the target class, wrap the
    existing method with a refined entry.

Because we always search for a refined marker in the class
hierarchy, we know which refined class to look for; this will fix
issues with refining a superclass of the target type, broken
currently.

When dispatching:

* Look for the method in the target class.
* If the entry is not marked as refined, return it.
* If the resulting entry is refined, search the refinement scope
  for a refined version of the method.
* If there's no refined version of the method in the refinement
  scope:
  * If the entry is a wrapper, return the wrapped method.
  * If the entry is a marker, continue the search up the
    hierarchy.
* This uses the owned class of the discovered unrefined method as
  the "super" class for the refinement, allowing supers to work
  from there.
Rework class hierarchy management for refinements.
This gets back to most functionality working plus a number of new
tests passing. The RefinedMarker instances are still leaking out
to various places, which is the cause of most of the regressed
tests.
Copy refinements into child scope on first use.
MRI does this in vm_cref_new, which I believe is called on first
entry to a method, block, or module body. In their case, they
reference the original overlay module and mark it as shared. Any
subsequent modifications cause it to be replaced on the parent
scopes. In JRuby, I am currently only propagating refinements into
class/module and method bodies, which means blocks and evals and
such still need to search up the hierarchy a bit. I am also doing
a hard copy of the related collection, which could add overhead to
boot time.
Temporary fix for our refined wrappers not actually wrapping.
MRI's logic for an existing method that gets refined expects that
it will be wrapped by a refinement marker. This allows the
original method to be pulled out and look unrefined for other
logic we also mimic. Since currently I do not wrap, but instead
flag, the original method, there's no way to get an original that
isn't refined without duping the method here. This is inefficient
and may lead to peculiar side effects so I will return to using
a full wrapper in a subsequent commit.
Make AsString an instruction with a proper call site.
This is necessary to handle refined to_s calls from interpolated
strings, as in MRI test_refinement.rb test_tostring.
Handle refinements within Symbol proc.
This logic modifies all calls with non-literal procs to pass the
current scope through, and modifies the coercion logic to handle
Symbol specially by passing the scope in. This will in turn use
refinements when present.

Note that this is not quite as optimal as the SymbolProc operand
but is still static for the creation of the proc.
Set refine block refinements to parent and search all the way.
This is not quite the right logic, since MRI will copy refinements
into each level and only search the one level. We do not
consistently do that, so we still search the scope hierarchy for
other refinements. This is inefficient and should change, but this
patch at least allows mutually-dependent refine blocks to see
each other.

headius added some commits Jan 31, 2019

@headius headius added this to the JRuby 9.2.7.0 milestone Feb 11, 2019

@enebo enebo merged commit cfd1427 into master Feb 11, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@headius headius deleted the refinements branch Feb 11, 2019

@headius headius restored the refinements branch Feb 12, 2019

@headius headius deleted the refinements branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.