+1
-Sundar
On 12/12/17, 7:09 PM, Hannes Wallnöfer wrote:
Thanks for the review, Sundar.
I think making thresholds configurable is a good idea. I’ve uploaded a new
webrev where SHARED_CALL_THRESHOLD and SHARED_GET_THRESHOLD are configurable
via „nashorn.shared.scope.call.threshold“ and
„nashorn.shared.scope.get.threshold“ system properties, respectively.
http://cr.openjdk.java.net/~hannesw/8069338/webrev.02/
Hannes
Am 12.12.2017 um 14:00 schrieb Sundararajan
Athijegannathan<sundararajan.athijegannat...@oracle.com>:
+1
PS. Not sure if it is worth making SHARED_GET_THRESHOLD configurable via System
property?
-Sundar
On 12/12/17, 5:51 PM, Hannes Wallnöfer wrote:
Thanks for the review, Attila.
I’ve uploaded a new webrev with your suggested changes (including making
ScriptObject.getProto(int) accept 0 argument and refactoring GET_PROTO code in
CodeGenerator into a method).
I have also done extensive performance testing over the last days and found
that for some reason, shared methods for variable access has a very detrimental
effect on performance for some reason. My theory is that we don’t save as much
by making property access shared, and the flood of additional methods has some
significant cost on codegen, runtime, JIT etc. Based on this I decided to
increase the threshold for shared property access again to 100, which performs
noticeably better than with the lower threshold in some benchmarks.
New webrev is here:
http://cr.openjdk.java.net/~hannesw/8069338/webrev.01/
Hannes
Am 11.12.2017 um 11:55 schrieb Attila Szegedi<szege...@gmail.com>:
Reviewing this now, thanks for going ahead and implementing it!
So what you are saying about slow scopes is that shared scopes were disabled
anyway with them for some time, so you didn’t really take away functionality,
just cleaned up the remains?
So, some remarks:
1. I see the bytecode shape you wrote around invocation of
ScriptObject.getProto(int), and I realized it’s weird it asserts n> 0 and it
has the first getProto() unrolled before the loop. I’m pretty sure it can just
look like this:
public final ScriptObject getProto(final int n) {
ScriptObject p = this;
for (int i = n; i--> 0;) {
p = p.getProto();
}
return p;
}
That assert was there because we knew that it’s only CodeGenerator that emits
calls to it, and it will always emit invocations with n> 0, but now we’d have
a justification for invoking it with n == 0, so I think it’s fine to do it and
avoid the no_proto_call branch in shared scope call bytecode. It’s not a big deal
even if you leave it as-is, though.
To be entirely pedantic, that assert in getProto should’ve read “n> 1” before
because the CodeGenerator was guaranteed to emit invocations to it with n> 1, so
a really pedantic implementation of it should’ve read:
public final ScriptObject getProto(final int n) {
assert n> 1;
ScriptObject p = getProto().getProto();
for (int i = n - 1; --i> 0;) {
p = p.getProto();
}
return p;
}
(God, I really hope everyone reading this realizes I’m joking with this last
one.)
FWIW, we could also factor out the:
if (depth> 1) {
method.load(depth);
method.invoke(ScriptObject.GET_PROTO_DEPTH);
} else {
method.invoke(ScriptObject.GET_PROTO);
}
pattern in CodeGenerator into its own little “emitGetProto(int depth)” method, as
it appears twice (and make sure it asserts on depth> 0 ;-) )
2. For that invocation of UnwarrantedOptimismException.withProgramPoint - I’d
create a Call objects using CompilerConstants.virtualCallNoLookup and stash it
in a static final field either in SharedScopeCall or
UnwarrantedOptimismException; it’s more typesafe than writing type descriptors
as strings.
3. So, UOE.hasPrimitiveReturnValue was unused, I take it?
3. In getStaticSignature you have another occurrence of number 3 that you can
replace with FIXED_PARAM_COUNT :-)
But these are all minor points… overall, this looks really good.
Attila.
On Dec 6, 2017, at 6:43 PM, Hannes Wallnöfer<hannes.wallnoe...@oracle.com>
wrote:
Please review:
Bug: https://bugs.openjdk.java.net/browse/JDK-8069338
Webrev: http://cr.openjdk.java.net/~hannesw/8069338/webrev.00/
This implements shared scope calls for optimistic vars/calls. I followed
Attila’s blueprint in the bug description to the last detail - thanks for
figuring it all out.
Some adjustments and cleanups I did along the way:
- Originally shared scope calls were designed to support slow scope vars
(with/eval). However, that feature was disabled later on. I refactored the code
to make it clear that we only do shared scope calls for fast scope operations.
- I unified the threshold for all shared scope operations to 5. I don’t think
there is a reason for different thresholds for different operations. I did some
testing, using 5 as threshold appeared to be a sensible value.
Running all of the octane benchmark I see a decrease in created callsites (200
000 to 178 000) and a slight decrease in callsite invalidations.
Hannes
