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 >
