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 <[email protected]>
> 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
