403815 - Implement fast Vector classes

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Erik Tierney]

Attachment: Implement Vector

This patch implements the Vector class from the ES4 proposal.  Note that it does not implement type-parameters, expecting the compiler to turn the type-parameter syntax for Vectors into concrete classes.  These concrete classes are optimized for int, uint, and Number.  Once type-parameters are implemented, we can go back and redo Vector that way.

The JIT has been modified to call fast get/set methods on the Vector classes.  This is similar to the optimizations done for Array.  The difference here is that for int, uint, or Number Vectors we know the specific type of the operation and can avoid unnecessary conversion to Atom and back (and between int and double when dealing with large ints).

This adds 4 new builtin types (Vector$int, Vector$uint, Vector$double, and Vector$object for the general case).  

I also added the errorgen script and neccessary .xml files that were missing from the hg repo.  I think these got left out from the initial cvs checkin, and no one has had to add error messages since.

Comment 1

[Edwin Smith]

Might be good to split the errorgen scripts into a separate bug/changeset.

Comment 2

[Erik Tierney]

Attachment: vector support, removed errorgen associated diffs

I created bug 403905 for the errorgen related changes, and generated a new diff for vector without those diffs.  The diff is much smaller now.

Comment 3

[Erik Tierney]

Attachment: Latest patch for Vector implementation

Comment 4

[Dan Schaffer]

Attachment: vector testcases in tests/es4/vector

add updated testcases for vector.  should all pass with latest patch.

Comment 6

[Erik Tierney]

Comment on attachment 293701 [details] [diff] [review]
vector testcases in tests/es4/vector

Looks good, I'll push these changes with the Vector changes.

Comment 7

[Edwin Smith]

Comment on attachment 293689 [details] [diff] [review]
Latest patch for Vector implementation

(In reply to comment #3)
> Created an attachment (id=293689) [details]
> Latest patch for Vector implementation

* should there be vector.<ScriptObject>, i.e. something designed for
subclasses of Object that arent one of the primitives?

* could conditional compilation used to implement castToThisType and the other pseudo-type-parameterization that's going on?  ie could the vector type be a config variable.  castToThisType certianly introduces some unwanted overhead

* castToThisType(this) should not be needed inside of a concrete method, only prototype methods

* should pop() return Object or *?  vector.<*> could contain undefined values right?  same for shift(), etc

* looks like some methods in template TypedVectorObject don't contain T in their signature or body.  should they move up to VectorBaseObject?  eg getUintProperty()

* should we remove flash.utils.IntArray, etc?  I dont think they are in flash and i dont know of anyone using them in tamarin standalone (maybe a separate ticket)

* in IntArray() constructor, this.fixed=fixed is commented out. bug?

(nit) Vector.as not in avmplus.vcproj

(nit) Verifier.cpp case OP_debugline, s/in debugger moed/in debugger mode/

Comment 8

[Erik Tierney]

I made some changes suggested, and pushed to tamarin central.  Responses below

(In reply to comment #7)
> (From update of attachment 293689 [details] [diff] [review])
> (In reply to comment #3)
> > Created an attachment (id=293689) [details] [details]
> > Latest patch for Vector implementation
> 
> * should there be vector.<ScriptObject>, i.e. something designed for
> subclasses of Object that arent one of the primitives?

I experimented with this a little.  I couldn't get any performance improvement by doing this instead of just storing Atom's.  I think the issue is that even if we get a ScriptObject* out of the Vector we usually have to call a helper to ensure its the right type we want, and that takes the majority of the time.

> 
> * could conditional compilation used to implement castToThisType and the other
> pseudo-type-parameterization that's going on?  ie could the vector type be a
> config variable.  castToThisType certianly introduces some unwanted overhead
> 

CC values can only be String, Numbers, or Booleans.  So there's no way to make a CC value be a type, like you can with the C preprocessor.  

> * castToThisType(this) should not be needed inside of a concrete method, only
> prototype methods

Removed the ones I found.

> 
> * should pop() return Object or *?  vector.<*> could contain undefined values
> right?  same for shift(), etc

Yeah, should be *, changed it.

> 
> * looks like some methods in template TypedVectorObject don't contain T in
> their signature or body.  should they move up to VectorBaseObject?  eg
> getUintProperty()

They have to call non-virtually methods that do use T, so I left them in TypedVectorObject.

> 
> * should we remove flash.utils.IntArray, etc?  I dont think they are in flash
> and i dont know of anyone using them in tamarin standalone (maybe a separate
> ticket)

Removed all the flash.utils.*Array types, and the C++ code which implemented them.  Nobody is using them.

> 
> * in IntArray() constructor, this.fixed=fixed is commented out. bug?
> 
Removed IntArray.

> (nit) Vector.as not in avmplus.vcproj
> 
> (nit) Verifier.cpp case OP_debugline, s/in debugger moed/in debugger mode/
> 

Cleaned these up.