Review request for JDK-8025435

[Attila Szegedi]

You have an awful lot of blank lines in import declarations in various files, and some import decls are not in alphabetical order, like all statically imported StringConstants in MethodGenerator.java.

s/Fucntion/Function
s/ocndition/condition
s/Contiunous/Continuous

Global.isBuiltinFunctionPrototypeApply and Global.isBuiltinFunctionPrototypeCall could be refactored into common code taking the name of the property as the parameter. Common code could probably also only invoke instance.getBuiltinFunction() once instead of three times in its body. In that case, maybe you also don't need the Global.isBuiltinFunction() method (less retrieval of thread locals)

Some comments in Global.extractBuiltinProperties would be helpful. Its name is somewhat misleading as it is both extracting the properties of built-in object, as well as the property of the Global for retrieving the object. It took me a bit to understand why it's doing what it's doing.

Global.java:2214 (reading tagBuiltinProperties("Global", this);) is commented out. Just remove it.

I'm somewhat wary of adding yet another field (the switch point) to Property. They're all over the place in large numbers. Maybe have a dedicated AccessorProperty subclass for built-in properties?

I know this is crazy, but what do we do when our IntArrayData reaches 2^31-1 elements and then we push an element? I think at least ArrayData.nextSize() should be checking for length overflows and throwing a TypeError.

A comment explaining that we relink on empty array in fast pop because we must return undefined would be great.

MethodHandleFactory.java has some System.err.printlns

It seems like no implementation of findFast* methods in ArrayData subclasses is using the receiver parameter.

Why do we need SpecializedFunction.linkCheckGetSwitchPoint? Why can't it just call ScriptObject.getModifiedSwitchPoint() directly once it established that the receiver is a ScriptObject?

I'm wary of adding a SwitchPoint field to every ScriptObject. Can't we make it so that only NativeArray has it? It's the only one that uses it, and it also seems like the usage is very special (only when making the length property non-writable). It seems like it's a very specific mechanism ("length made non writable") disguised as a generic one ("some modification"). That doesn't feel right. I'd go as far as to say that I wouldn't even keep it as a field, maybe externalize it in a WeakHashMap<NativeArray, SwitchPoint> and create it on-demand.

Shouldn't ScriptObject.findBuiltinSwitchPoint() return null as soon as it encounters an invalidated switchpoint? Currently, it'll keep traversing the prototype chain even if it finds an invalidated switchpoint.

It doesn't feel right that SpecializedFunction has a bunch of knowledge about its users: Guard enums are highly specific to Array.push/pop and String.charAt, as is the LinkCheck. It feels like the specific knowledge in Guards and LinkCheck should live in NativeArray (or ArrayData, or ContinuousArrayData) and NativeString classes instead. It feels like breaking of encapsulation and another case of a specific mechanism presented as a more general one. Is this something that must be done this way for sake of annotation processing?

CompiledFunction.isOptimistic is not used.

Specialization.isOptimistic is only used to bind a fixed FIRST_PROGRAM_POINT value to the first argument of invoker in CompiledFunction. Can you explain this to me? I've looked at the Specialization.optimistic documentation, and I'm not clear how it helps to bind a FIRST_PROGRAM_POINT to a method that can throw an UOE. Also, I don't see anything currently invoking any other constructor than Specialization(MethodHandle), but there might be some codegen stuff going on in the nasgen that I haven't completely grasped yet.

I might still look at things, but wanted to throw out things so far out here.

Attila.


On Sep 22, 2014, at 4:38 PM, Marcus Lagergren <marcus.lagergren at oracle.com> wrote:

> Please review JDK-8025435 at http://cr.openjdk.java.net/~lagergren/8025435/
> 
> https://bugs.openjdk.java.net/browse/JDK-8025435
> 
> This is the framework for optimistic builtin functions. Summary of work
> 
> * Introduced SpecializedFunction and SpecializedFunction.Guard for fast optimistic implementations
> * Modified ScriptFunction so that it picks the best specialization first, and checks if it can link using the above datastructures
> * Modified Nasgen to recognize the SpecializedFunction and its annotations
> * Implemented fast versions of Array.push/pop and String.charCodeAt as proof of concepts
> * Added a switchpoint based rather than guard based framework for invalidation of builtin functions (on a per context basis), to get rid of previous guard overhead
> * Added primitive linkage without proto filter as long as builtins acting upon them haven’t been rewritten
> * Currently there is support for invalidation of both entire builtins e.g. Array.prototype = something and proerties Array.prototype.push = something, but the granularity right now, to save switchpoints, uses the same switchpoint for an entire builtin and all its properties, as any other cases are rare. Granularity can easily be increased by adding keys to the builtinSwitchPoints table in the Context
> * Prefer to invalidate callsite by ClassCastException and failed type checks instead of explicit guards.
> 
> I’m saving further specializations for later dates.
> 
> Added various microbenchmarks to prove performance of the implementations of the current functions.
> 
> Before patch:
> 
> zann:make marcus$ sh ../bin/runopt.sh ../test/examples/push-pop-benchmark.js
> 18997 ms
> 
> Verified OK - result is correct
> 
> After patch:
> 
> zann:make marcus$ sh ../bin/runopt.sh ../test/examples/push-pop-benchmark.js
> 2327 ms
> 
> Verified OK - result is correct
> zann:make marcus$ d8 ../test/examples/push-pop-benchmark.js
> 9672 ms
> 
> Verified OK - result is correct
> 
> Similar benchmark exists for charCodeAt - my other proof of concept, which runs about 5x faster too. 
> 
> Test and test262 are clean after some horror corner cases with lengths and array like objects.
> 
> Regards
> Marcus
> 
>