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
