I personally don’t believe anybody will ever be fidgeting with these, but it doesn’t hurt having them configurable, so… +1 :-)
> On Dec 12, 2017, at 2:39 PM, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> > wrote: > > 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 >
