+1 from me. This took a while, but it's a significant chunk of work – I'm glad we could beat it into its final shape.
On Sep 25, 2014, at 1:50 PM, Marcus Lagergren <[email protected]> wrote: > Final version at > > http://cr.openjdk.java.net/~lagergren/8025435.3/ > > (with the small exception that JDK-8021122.js is changed in the webrev with > leftover bug printouts still there. I have removed those) > > Ant test passes. > As far as I understand I have Attila’s and Hannes’s blessing to move this to > 9, and hopefully we can sort if it should go to 8u40 today. Attila and Hannes > have both contributed compelling arguments for this. > Also https://bugs.openjdk.java.net/browse/JDK-8044689 is easily fixable WITH > this infrastructure, but not without it. > > > /M > > On 25 Sep 2014, at 12:48, Marcus Lagergren <[email protected]> > wrote: > >> >> On 25 Sep 2014, at 11:50, Attila Szegedi <[email protected]> wrote: >> >>> MemberInfo.java: s/thre/the >>> BufferArray.java: s/Constructur/Constructor >>> >>> ApplySpecialization: Set<Expression> argumentsFound could be a boolean >>> instead of a set. >>> >> I need a writable datastructure that is still final outside the loop. I’ll >> leave this for now. Might just as easily have done final boolean result[] = >> new boolean[1] or something… also ugly. >> >>> I asked for comments in Global.extractBuiltinProperties; I don't see them >>> having been added. >>> >> >> Added >> >>> Not directly part of this (but rather earlier work leading up to this), but >>> I think Global.BOOTSTRAP is a very misleading name. Also, the whole >>> mechanism to invokedynamic through INVALIDATE_NAME_BOOTSTRAP seems too >>> roundabout to me. Seems to me we could do a straight invokestatic to >>> Global.invalidateReservedName(). >>> >> Done. All that logic gone. See below. >> >>> BTW: after thinking about this I noticed that >>> Global.invalidateReservedName() is an instance method, not a static method. >>> I can see you bind INVALIDATE_RESERVED_NAME to a Global. Two remarks about >>> that: (1) instance fields shouldn't be named to look like static constants >>> (2) This all gives the mistaken impression that invalidation is still >>> per-global instead of per-context. Really, the whole invokedynamic through >>> Global.invalidateNameBootstrap should be replaced with an INVOKESTATIC to a >>> method in ScriptRuntime (which is meant to be our primary script-to-runtime >>> API), and Global should have no traces of this logic; it's not the place >>> for it. >>> >> Fixed. All the logic is moved to a static call in ScriptRuntime, as it >> should be. Thanks for spotting this. This is actually legacy from my first >> implementation, before I fixed this for all builtin properties. >> >>> That phrase "because one of the thinks" in a comment to NativeArray. >>> setIsLengthNotWritable makes no sense; needs some proofreading. >>> NativeArray.popInt() JavaDoc has some stray " + " markers in it. Method's >>> exceptions should be declared in a @throws, not @return >>> NativeArray.pushInt() JavaDoc: @throws vs. @return too; some others too >>> >> Fixed. >> >>> Does LinkLogic have to have a cache for the Switchpoints? I'm asking >>> because if it didn't, then you also wouldn't have to have two fields for >>> PushLinkLogic and PopLinkLogic in NativeArray, as you wouldn't have to >>> cache those either but could just always create new instances in >>> getLinkLogic. Just a thought – I can accept that you can have genuine >>> additional reasons to cache switchpoints in LinkLogic. >>> >> No we don’t need that cache performance wise, but I currently don’t want to >> create new link logic instances per callsite I link for push and pop. I am >> reluctant to change this now and have to redo all performance measurements >> so I’d rather leave it for later, if I may. I don’t think it matters, but I >> don’t want to cause any last minute problems. >> >>> FinalScriptFunctionData: why are addInvoker methods now returning >>> CompiledFunction instead of void? Doesn't seem to be used. >>> >> Left it like that too, I used them for constructing invoker lists in the >> beginning, but not right now, but I see no harm in exposing the return >> invokers as return values, it makes a more flexible API possible where you >> can chain addInvoker calls for example >> >>> GlobalFunctions: admittedly a matter of taste/style, but all those constant >>> and identity method handles; I might've just declared them as small private >>> static Java methods instead, e.g. public static int parseInt(Object this, >>> int i) { return i; } instead of playing with combinators. Works either way, >>> just we end up with more lambdaforms… >>> >> I left it like it is, because it is consitent to the rest of the >> specializations right now. >> >>> ScriptObject.isProto() seems unused. In any case should be named hasProto… >>> >> Removed >> >>> Why not remove ScriptObject.checkReservedName? >> >> Removed >>> >>> That's all. >>> Attila. >>> >>> On Sep 24, 2014, at 2:31 PM, Marcus Lagergren <[email protected]> >>> wrote: >>> >>>> OK, new webrev addressing Attila’s comments and Sundar’s and Hannes’s >>>> offline comments as well is up at : >>>> http://cr.openjdk.java.net/~lagergren/8025435.2/ >>>> >>>> I’ve now moved, as suggested, all abstraction about various optimistic >>>> strategies into the native objects themselves, which indeed cleaned stuff >>>> up nicely. The result is shorter overall codemass. The problem with >>>> primitives and optimistic builtins was present in the old branch itself, >>>> only it did not manifest itself. >>>> >>>> Test is clean. Leaving to pick up the kids now, and will run test262 and >>>> octane when I am home with the goal of checking that everything is correct >>>> and verifying my octane performance improvements on a dedicated >>>> benchmarking machine. >>>> >>>> Regards >>>> Marcus >>>> >>>> >>>> On 23 Sep 2014, at 17:36, Marcus Lagergren <[email protected]> >>>> wrote: >>>> >>>>> Guys, guys! This is my review thread for JDK-8025435. Start another one >>>>> for this, please… >>>>> >>>>> /M >>>>> >>>>> On 23 Sep 2014, at 17:29, Aleksey Shipilev <[email protected]> >>>>> wrote: >>>>> >>>>>> On 09/23/2014 05:58 PM, Thomas Wuerthinger wrote: >>>>>>> I understood your remark. I just believe that this restriction is not >>>>>>> fundamental to JMH and there *could* be support for languages that do >>>>>>> not statically compile to Java bytecodes as well. >>>>>> >>>>>> I think this thread side-tracks. >>>>>> >>>>>> Bottom-line: There are only two ways to hook up dynamically-compiled >>>>>> language runtime into JMH (say, JS engine), depending who is in control: >>>>>> >>>>>> a) Ask JS engine to start executing the JS benchmark, *and* call back >>>>>> into JMH with an AST/bytecode/whatever in hands, asking to measure it. >>>>>> We would need to generate the synthetic Java code, compile it, load it, >>>>>> and only then run it, calling back to JS runtime for execution of our >>>>>> payload -- all in flight, and requiring the tight cohesion. >>>>>> >>>>>> b) Ask JMH to compile and execute a Java benchmark, and call into JS >>>>>> engine with a JS script in hands, asking to compile and run it. There, >>>>>> users statically compile the benchmark JAR, and it runs with minimal >>>>>> dynamic dances, also going through the standard APIs (javax.script). >>>>>> >>>>>> While there is nothing fundamental preventing us from exploring the >>>>>> route (a), it is not as practical as going the well-established route >>>>>> (b). In other words, "could be done" != "should be done". >>>>>> >>>>>> -Aleksey. >>>>>> >>>>> >>>> >>> >> >
