RFR: 8069338: Implement sharedScopeCall for optimistic types

[Attila Szegedi]

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.wallnoefer at 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