Remove premade field-based DynamicScope and generate instead. #4174

headius · 2016-09-23T16:52:52Z

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.

headius · 2016-09-23T16:55:41Z

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.

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.

headius · 2016-09-23T19:40:59Z

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

Classloading is complicated. JIT code needs to load into a classloader that can see all scopes that code will try to access. Since we can't have multiple one-shot classloaders as parents, we would need to put all specialized scopes into a common classloader. This in turn keeps them from GCing. That may not be a huge issue, though...a given application will always have a finite, probably-small set of scope sizes.
In order to access the fields, all accesses must checkcast. Unknown what impact this has on performance yet. This could be mitigated by knowing the scope types for all accessed variables and doing the cast once. It can also be statically created with the right type if it is the depth zero scope, negating the need for casts.

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.

headius · 2016-09-28T19:14:08Z

@subbuss @enebo @kares I'm pretty much done with this PR and would like input and review.

The concerns I had/have are:

Generating too many classes. This is mitigated by a limit of 50 in my most recent commit.
Cost of construction. This has been mitigated by using MethodHandle instead of reflection and caching that handle in StaticScope, so there's fewer steps from newDynamicScope to having an object in hand.
Caching classes statically. This is still done, but since there will never be more than 50 within the classloader that booted JRuby, I am not worried.

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.

headius · 2016-09-28T19:16:07Z

Oh, this is also partially done on master too, but there's only two hand-written dynamic scope classes now: DynamicScope and ManyVarsDynamicScope.

enebo

Two trivial comments. My only reservation I think we will need to wait and see...we keep increasing our dependence of invokedynamic.

enebo · 2016-09-29T19:31:34Z

core/src/main/java/org/jruby/ir/targets/JVMVisitor.java

+        String baseName = p(DynamicScope.class);
+
+        if (depth == 0) {
+


extra newlines in this method

The newlines existed before the PR too. May be unnecessary with the simpler body, though.

enebo · 2016-09-29T19:40:09Z

core/src/main/java/org/jruby/runtime/scope/ManyVarsDynamicScope.java

@@ -43,6 +53,7 @@ private void allocate() {
    }

    public DynamicScope cloneScope() {


We can deprecate all cloneScopes since I do not see them used anywhere (even without this PR)

Oh, very nice. Ok.

…opes

headius force-pushed the generated_dynamicscopes branch from a8eece9 to bb41cc7 Compare September 23, 2016 17:14

headius force-pushed the generated_dynamicscopes branch from bb41cc7 to c9370c2 Compare September 23, 2016 17:25

headius added 8 commits September 23, 2016 14:49

Remove Dummy and NoVars scope in favor of generated 0-var scope.

2403780

Use method handles for construction of scope types.

6049fe1

Merge branch 'master' into generated_dynamicscopes

838fd07

Generate offset-specialized methods for dynscope subs.

eb890dc

Expand specialization of generated DynamicScope.

8a7f32a

* Get and set of depth zero, offset = 0..9 * Flip override of sets to void versions * Add test for the key methods

Merge branch 'master' into generated_dynamicscopes

2dffd52

Merge branch 'master' into generated_dynamicscopes

496c133

headius added this to the JRuby 9.1.6.0 milestone Sep 28, 2016

headius added core jit performance JRuby 9000 labels Sep 28, 2016

Add BUG to clone failure re-raise and explain ManyVars not cloning

f261580

headius force-pushed the generated_dynamicscopes branch from f4b539c to ba3e3f2 Compare September 28, 2016 19:43

Fix moved field reference and clean up imports.

60241ad

headius force-pushed the generated_dynamicscopes branch from ba3e3f2 to 60241ad Compare September 28, 2016 20:25

enebo approved these changes Sep 29, 2016

View reviewed changes

headius added 5 commits September 29, 2016 14:54

Wrong signature for specialized sets.

8ce1238

Clean up set code a bit.

380e42a

Tidy up some excess whitespace.

b561339

Mark DynScope methods used by jit with @jit.

1b657df

Deprecate DynamicScope.cloneScope.

06a0d8b

headius added 3 commits September 29, 2016 15:53

Update for jitescript 4.1.

1a7d10a

Merge remote-tracking branch 'origin/master' into generated_dynamicsc…

3156979

…opes

Use jitescript 0.4.1.

7d22b20

headius merged commit 7d22b20 into jruby:master Sep 30, 2016

headius deleted the generated_dynamicscopes branch September 30, 2016 03:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove premade field-based DynamicScope and generate instead. #4174

Remove premade field-based DynamicScope and generate instead. #4174

headius commented Sep 23, 2016

headius commented Sep 23, 2016

headius commented Sep 23, 2016

headius commented Sep 28, 2016

headius commented Sep 28, 2016

enebo left a comment

enebo Sep 29, 2016

headius Sep 29, 2016

enebo Sep 29, 2016

headius Sep 29, 2016

		@@ -43,6 +53,7 @@ private void allocate() {
		}

		public DynamicScope cloneScope() {

Remove premade field-based DynamicScope and generate instead. #4174

Remove premade field-based DynamicScope and generate instead. #4174

Conversation

headius commented Sep 23, 2016

headius commented Sep 23, 2016

headius commented Sep 23, 2016

headius commented Sep 28, 2016

headius commented Sep 28, 2016

enebo left a comment

Choose a reason for hiding this comment

enebo Sep 29, 2016

Choose a reason for hiding this comment

headius Sep 29, 2016

Choose a reason for hiding this comment

enebo Sep 29, 2016

Choose a reason for hiding this comment

headius Sep 29, 2016

Choose a reason for hiding this comment