+1
> On Dec 12, 2017, at 1:21 PM, Hannes Wallnöfer <hannes.wallnoe...@oracle.com>
> 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
>>
>
