573452 - Type-specialized Vector methods (AS3 implementations)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect, P3)

Description

[Lars T Hansen]

(For some background see bug #477139.)

The AS3-implemented vector methods are written in a type-unspecialized way.  There may be some benefits to specializing them to specific vector types, since we'd get early binding that way.  In the 'push' method there are several accesses to 'this' but the code has no knowledge of what its type is.  To do better we'd need to specialize 'push' for each of the Vector implementation types.

Comment 1

[Sandeep]

Attachment: Benchmark files to test vector push,join,shift, toLocaleString

Comment 2

[Sandeep]

Attachment: Patch to Type-specialize vector push,join,shift and toLocaleString.

Following is the comparison between type specialize vector and untyped vector for different types
type	push typed	push untyped
		
uint 	45	        39
int	45	        39
double	42	        37
object	18	        17

type	join typed	join untyped
		
uint 	10	        10
int	10	        10
double	4	        4
object	4	        4

type	shift typed	shift untyped
		
uint 	3	        3
int	3	        3
double	1	        1
object	2	        2

type	toLocaleString typed  toLocaleString untyped
		
uint 	9	              9
int	9	              9
double	4	              4
object	6	              6

Only push seems to have performance improvement.

Comment 3

[Sandeep]

Comment on attachment 473405 [details]
Benchmark files to test vector push,join,shift, toLocaleString

Need feedback whether benchmark files are proper or not.

Comment 4

[Sandeep]

Comment on attachment 473409 [details] [diff] [review]
Patch to Type-specialize vector push,join,shift and toLocaleString.

Please refer comment #2 for results I got.

Comment 5

[Steven Johnson]

Comment on attachment 473405 [details]
Benchmark files to test vector push,join,shift, toLocaleString

Do these actually work? I don't think that

        var a = new <int>[];

is legal syntax, it should be

        var a = new Vector.<int>[];

Likewise, you can't create a new Vector with a literal, as you can with an array, so

    var a = new <int>[0,1,2,3,4,5,6,7,8,9];

won't work.

*Conceptually* this looks fine, though, but please submit tested code next time :-)

Some other minor nits:

You should include asmicro/driver.as rather than replicating the TEST3 function though. 

Two style isseus:

-- all new code should use spaces, not tabs. 4 spaces is preferred indentation.

-- preferred style in AS3 is

    function foo() 
    {

rather than

    function foo() {

Comment 6

[Steven Johnson]

Comment on attachment 473409 [details] [diff] [review]
Patch to Type-specialize vector push,join,shift and toLocaleString.

You don't need to do

    var v:Vector$object = this;

in the specialized versions; we already know that 'this' is a Vector$object (or whatever), you should be able to just use 'this' directly. Otherwise, the patch looks plausible with minor objections noted below.

The loop structure is awkward, but I guess that's how the previous code was... feels like it could be tightened, but this isn't essential.

Also, see the style nits from the other patch on AS3 formatting and tab usage; additionally, please prefer the style

    if (cond)
    {

to 

    if( cond ) {

and insert a space between // and the beginning of the comment text.

Comment 7

[Jimmy Bahuleyan]

(In reply to comment #5)
> Comment on attachment 473405 [details]
> Benchmark files to test vector push,join,shift, toLocaleString
> 
> Do these actually work? I don't think that
> 
>         var a = new <int>[];
> 
> is legal syntax, it should be
> 
>         var a = new Vector.<int>[];
> 
> Likewise, you can't create a new Vector with a literal, as you can with an
> array, so
> 
>     var a = new <int>[0,1,2,3,4,5,6,7,8,9];
> 
> won't work.

Both

var a = new <int>[0,1];
var a = new Vector.<int>[0,1];

passes through strict mode compilation, the generated abc is a bit different (don't know why though). Is there a reason we might prefer one over the other for benchmarks (Vector.<int> looks like the faster one)?

Comment 8

[Lars T Hansen]

(In reply to comment #5)
> Comment on attachment 473405 [details]
> Benchmark files to test vector push,join,shift, toLocaleString
> 
> Do these actually work? I don't think that
> 
>         var a = new <int>[];
> 
> is legal syntax, it should be
> 
>         var a = new Vector.<int>[];

That's not true.

The vector literal syntax is 'new <int>[E1, ..., En]' and has been supported for some time.

Jimmy, please note carefully that

  new Vector.<int>[1,2,3]

*does not mean what you think it means*.  Specificially, it parenthesizes like this:

  (new Vector.<int>)[1,2,3]

which is to say, fetch the element named "3" from a new vector containing no elements (and will result in a range error exception being thrown if you try to run it).

Comment 9

[Lars T Hansen]

I should add that the Vector.<T> constructor takes zero or one arguments; if one argument is provided it needs to be the length of the vector.

You can also convert an Array instance to Vector:

  Vector.<int>([1,2,3])

It is a known performance bug (https://bugzilla.mozilla.org/show_bug.cgi?id=532690#c16) that this is actually faster than the equivalent

  new <int>[1,2,3]

Comment 10

[Sandeep]

(In reply to comment #6)
> Comment on attachment 473409 [details] [diff] [review]
> Patch to Type-specialize vector push,join,shift and toLocaleString.
> 
> You don't need to do
> 
>     var v:Vector$object = this;
> 
> in the specialized versions; we already know that 'this' is a Vector$object (or
> whatever), you should be able to just use 'this' directly. 

But https://bugzilla.mozilla.org/show_bug.cgi?id=477139#c10 says that 'this' is untyped if we use it directly. So we have to assign to appropriate typed variable. Am I missing anything here?

Comment 11

[Sandeep]

Attachment: Benchmark file for type specialized vector push

uses driver.as and style changes.

Comment 12

[Sandeep]

Attachment: Patch for Type Specialized vector push

Fixed all comments from Steven Johnson except removal of 
var v:Vector$object = this. Without reassigning 'this' there is no performance benefit. 

Following are the untyped vs typed performance results:
                 untyped               typed
uint             37                    45  
int              39                    45 
double           36                    43
object           17                    18

We didnt see any performance improvement for any other vector functions. So this patch only has changes for vector.push.

Comment 13

[Sandeep]

Attachment: Patch for Type Specialized vector push

Fix problem with previous attachment.

Comment 14

[Edwin Smith]

Comment on attachment 478208 [details] [diff] [review]
Patch for Type Specialized vector push

A comment on each copy mentioning that any changes should be propagated to the other copies, would be useful.

   var v:Vector$object = this;
         ^^^^^^^^^^^^^

It's odd to see type annotations like this in AS3.  Should it be v:Vector.<Object> instead?  Is the bytecode the same either way?

The benchmark #s look promising so far.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #10)
> (In reply to comment #6)
> > Comment on attachment 473409 [details] [diff] [review] [details]
> > Patch to Type-specialize vector push,join,shift and toLocaleString.
> > 
> > You don't need to do
> > 
> >     var v:Vector$object = this;
> > 
> > in the specialized versions; we already know that 'this' is a Vector$object (or
> > whatever), you should be able to just use 'this' directly. 
> 
> But https://bugzilla.mozilla.org/show_bug.cgi?id=477139#c10 says that 'this' is
> untyped if we use it directly. So we have to assign to appropriate typed
> variable. Am I missing anything here?

FYI, after reading the dialogue on this bug, I figured it made sense to file a bug against ASC, since Steven's reasoning sounded good to me:

  https://bugs.adobe.com/jira/browse/ASC-4138

Comment 16

[Steven Johnson]

(In reply to comment #10)
> But https://bugzilla.mozilla.org/show_bug.cgi?id=477139#c10 says that 'this' is
> untyped if we use it directly. So we have to assign to appropriate typed
> variable. Am I missing anything here?

In those cases that is true, since 'this' is the base class, not the specialized class. But in this case the method lies in the specialized class, so 'this' is known to be that class. (Or should be -- if not, ASC is doing something mighty dumb here.)

(In reply to comment #15)
> FYI, after reading the dialogue on this bug, I figured it made sense to file a
> bug against ASC, since Steven's reasoning sounded good to me:

Ouch! Ignore my comment above, I guess -- seems mighty weird for 'this' to be untyped, but if ASC is compiling that way, then my comment is clearly wrong. (It's worth adding comments to the code to this effect, in case someone else makes the same assumption I did....)

Comment 17

[Steven Johnson]

Comment on attachment 478208 [details] [diff] [review]
Patch for Type Specialized vector push

Probably should add a comment about why the explicit "var v:Vector$object = this;" is necessary (ie, compiler issue)

Comment 18

[Sandeep]

(In reply to comment #14)
> Comment on attachment 478208 [details] [diff] [review]
> Patch for Type Specialized vector push
> 
> A comment on each copy mentioning that any changes should be propagated to the
> other copies, would be useful.
> 
>    var v:Vector$object = this;
>          ^^^^^^^^^^^^^
> 
> It's odd to see type annotations like this in AS3.  Should it be
> v:Vector.<Object> instead?  Is the bytecode the same either way?
> 
> The benchmark #s look promising so far.

The '$' syntax is used through out "vector.as" file, i.e why, I also used the same syntax. But compiler is allowing this syntax only in the vector builtin class. If I use '$' syntax in any other user classes, conmpiler is generating error. What is the difference between the two syntaxes?  Should I change it to Vector.<Object>?

Comment 19

[Sandeep]

Attachment: Patch to Type-specialize vector push

Incorporated Edwin's and Steve's comments. No one has come back on the vector.<Object> vs vector$object syntax. So, that is unchanged.

Comment 20

[Werner Sharp]

Comment on attachment 480885 [details] [diff] [review]
Patch to Type-specialize vector push

Looks good to me.

Comment 21

[Sandeep]

(In reply to comment #20)
> Comment on attachment 480885 [details] [diff] [review]
> Patch to Type-specialize vector push
> 
> Looks good to me.

(In reply to comment #20)
> Comment on attachment 480885 [details] [diff] [review]
> Patch to Type-specialize vector push
> 
> Looks good to me.

Is the patch is ok to check-in or should it go through the review and super review. Anyways I dont have check-in rights. If it is ready for check-in, can anyone do it for me.

Comment 22

[Trevor Baker]

To Steven to land.

Comment 23

[Tamarin Bot]

changeset: 6563:0c942416e68f
user:      Lars T Hansen <[e-mail]>
summary:   Fix 573452 - Type-specialized Vector methods (AS3 implementations), slightly cleaned up and re-tested (patch=sakula, f=wsharp, f=stejohns, push=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/0c942416e68f

Comment 24

[Brent Baker]

QE: this needs test media to make sure that the code paths are touched/working