Review request for JDK-8025435

[Marcus Lagergren]

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 <marcus.lagergren at oracle.com> wrote:

> 
> On 25 Sep 2014, at 11:50, Attila Szegedi <attila.szegedi at oracle.com> 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 <marcus.lagergren at oracle.com> 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 <marcus.lagergren at oracle.com> 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 <aleksey.shipilev at oracle.com> 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.
>>>>> 
>>>> 
>>> 
>> 
>