-
-
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
Create a different hierarchy of DynamicScope for use in startup interp + remove null checks in the existing hierarchy since they are redundant #4167
Comments
Some ideas from discussing with Tom:
|
My thought as well. |
Our IR performes liveness checks on variables and should never emit code that accesses variable indices outside the prescribed scope size. This patch adds new getters that do not perform these unnecessary checks, and wires up the JIT to call them. This patch does not modify Full interpretation to use these unchecked paths, because it would require either a boolean in LocalVariaable operand and LoadLocalVariable instruction or a pass to replace them with "Unchecked" versions. It would also be better if we could generate these shapes of DynamicScope on the fly, rather than maintaining a finite set of mostly-identical subclasses. See jruby#4167.
I've merged my PR that adds unchecked get* paths to all DynamicScope types. It also removes a scope type we no longer used, and fixes a null leak from Thread#name uncovered by the DynamicScope changes. Note also that this PR only uses the unchecked paths from JIT, since Full IR still needs a way to indicate that a LocalVariable access should be unchecked. Currently LocalVariable.retrieve uses DynamicScope.getValue(offset, depth) and does its own null checking, and I believe some of the interpreter logic short-circuits the retrieve call and goes after DynamicScope directly. |
@enebo How/where should we change full IR or full interpreter to use the unchecked paths? |
@headius I can change it. It is retrieveOp in InterpreterEngine but I need to make sure Startup is not using that version. |
You guys both asked me about perf. Here's a trivial bench:
This is a pretty good result, considering the loop itself takes a good portion of this:
So the three-variable swapping is maybe 0.37s on master+#4168 and 0.55s on 9.1.5.0, so that's around a 30% improvement. Not bad! |
@headius nice! For those who follow our issues and want a little more background on impact...We try to eliminate heap variable scopes as much as we can. If a method has a closure in it then we end up generating a variable scope since we do not generally know whether that closure could end up becoming a binding. This change ends up improving variable access times whenever we have a heap scope. So if a method contains a closure it will likely see this improvement (it obviously may get watered down based on what else you do in that method). We do also create scopes for other reasons (like some native methods) but mainly you will notice the improvement in methods containing blocks. |
For fun I ran the microbench against 1.7 with and without indy on and we are twice as fast in both modes as 1.7 now. I am not sure if that is because of our IR opts or because we now have some mandatory indy stuff wired in on 9k. Still, it is fun to see us improving relative to 1.7.x. |
Yes, good results. Better than I anticipated even. |
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.
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.
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.
This is not addressed in full interpreter but I am closing this as resolved since we have it where it matters (JIT) and I want it in 9.1.6.0 issue list. |
The runtime now has a AddMissingInitsPass that the full interp and JIT modes use to add any required initializations of variables. This eliminates the need to do the same in DynamicScopes when lvars are looked up and they can be removed which might potentially lead to some perf. improvements by making DynamicScope code lean.
However, the startup interpreter won't have these IR-added inits in place and will still need the null checks. A good way to do that is add a different hierarchy there, and push those dynamic scopes for IR scopes that are running in startup mode. Maybe even a single 'multi-var-dynamic-scope' is sufficient for starters to evaluate what this yields. If startup impact is bad, the single scope can be specialized for different scenarios.
The text was updated successfully, but these errors were encountered: