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 <[email protected]>
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
>
>
