-
-
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
Remove premade field-based DynamicScope and generate instead. #4174
Conversation
It might also be possible to implement getValue and setValue using Unsafe rather than switches, since Unsafe could provide direct access to the field without any branching logic. |
a8eece9
to
bb41cc7
Compare
Instead of having hand-written DynamicScope subtypes that use fields instead of an array, this logic generates those subclasses as needed. Currently it only overrides the base getValue and setValue methods and DynamicScope now implements the other previously-abstract methods by calling those two. This is not optimal for generic access via those indexed methods, but the expectation is that JIT can just go after the fields directly. The additional overrides would likely be useful for the interpreter, though. See jruby#4167.
bb41cc7
to
c9370c2
Compare
I did a prototype of directly accessing fields from the specialized scopes. It worked reasonably well, but there's a few snags. It does not boot properly (because I removed ManyVarsDynamicScope and other classes that are actually still needed), but it does generate the expected code. https://gist.github.com/headius/2be4be6b1b8bc71f714901519ee6b5b0
The comparative bytecode is here: https://gist.github.com/headius/2c0b2459d8e5fb3c69bbaec254295ea8 This should have better performance than any previous mechanism, unless the checkcast is killing us. Just an experiment for now. |
* Get and set of depth zero, offset = 0..9 * Flip override of sets to void versions * Add test for the key methods
* Limit specialized subclasses to 50 wide. * Move to a generator class. * Cache construction logic to StaticScope. * Cache constructor in StaticScope. * Reduce duplication in code gen. * Move offset error to a shared method in generated classes. * Use a single OneShotClassLoader since we never dereference. * Misc perf, style tweaks.
@subbuss @enebo @kares I'm pretty much done with this PR and would like input and review. The concerns I had/have are:
The biggest improvement in this PR from beginning to end is that there are now ten offset-specialized get/set methods that are generated for all specialized DynScopes, and the JIT will automatically use them. Adding more requires only implementing them in DynamicScope and ManyVarsDynamicScope and adding their names to the lists in DynamicScopeGenerator. These methods allow heap variable accesses to inline down to a single field read or write with no extra overhead. Inlining-wise at the JVM level, a given jitted code body will generally have only one StaticScope and will only ever see one DynamicScope type, so every method body should be able to inline logic for its own scope. I have not revisited the direct field access because I was able to expand the use of offset-specialized methods. However, it might be interesting to see indy-based scope var access, which could specialize beyond the base set of offsets. I have no plans to do this right now. |
Oh, this is also partially done on master too, but there's only two hand-written dynamic scope classes now: DynamicScope and ManyVarsDynamicScope. |
f4b539c
to
ba3e3f2
Compare
ba3e3f2
to
60241ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two trivial comments. My only reservation I think we will need to wait and see...we keep increasing our dependence of invokedynamic.
String baseName = p(DynamicScope.class); | ||
|
||
if (depth == 0) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra newlines in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newlines existed before the PR too. May be unnecessary with the simpler body, though.
@@ -43,6 +53,7 @@ private void allocate() { | |||
} | |||
|
|||
public DynamicScope cloneScope() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can deprecate all cloneScopes since I do not see them used anywhere (even without this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, very nice. Ok.
Instead of having hand-written DynamicScope subtypes that use
fields instead of an array, this logic generates those subclasses
as needed.
Currently it only overrides the base getValue and setValue methods
and DynamicScope now implements the other previously-abstract
methods by calling those two. This is not optimal for generic
access via those indexed methods, but the expectation is that JIT
can just go after the fields directly. The additional overrides
would likely be useful for the interpreter, though.
See #4167.