Bug 128788 – Implement a few more Array prototype functions in JS

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Implement a few more Array prototype functions in JS

[Oliver Hunt]

Created attachment 224141 [details]
Patch

[Build Bot]

Comment on attachment 224141 [details]
Patch

Attachment 224141 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5978469586436096

New failing tests:
inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html

[Build Bot]

Created attachment 224156 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Ryosuke Niwa]

DYEBench A/B testing results:

Patched Average: 9118.3 ms (2.5% faster!)
Unpatched Average: 9347.3 ms

Patched:
Iteration 1: 9328 ms
Iteration 2: 9026 ms
Iteration 3: 9093 ms
Iteration 4: 8986 ms
Iteration 5: 9045 ms
Average: 9095.6 ms

Unpatched:
Iteration 1: 9431 ms
Iteration 2: 9237 ms
Iteration 3: 9263 ms
Iteration 4: 10064 ms
Iteration 5: 9383 ms
Average: 9475.6 ms

Patched:
Iteration 1: 9279 ms
Iteration 2: 9033 ms
Iteration 3: 9140 ms
Iteration 4: 9144 ms
Iteration 5: 9054 ms
Average: 9130 ms

Unpatched:
Iteration 1: 9417 ms
Iteration 2: 9215 ms
Iteration 3: 9314 ms
Iteration 4: 9208 ms
Iteration 5: 9328 ms
Average: 9296.4 ms

Patched:
Iteration 1: 9395 ms
Iteration 2: 9137 ms
Iteration 3: 8964 ms
Iteration 4: 9072 ms
Iteration 5: 9079 ms
Average: 9129.4 ms

Unpatched:
Iteration 1: 9328 ms
Iteration 2: 9118 ms
Iteration 3: 9295 ms
Iteration 4: 9300 ms
Iteration 5: 9309 ms
Average: 9270 ms

[Gavin Barraclough]

Comment on attachment 224141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224141&action=review

r- due to defineOwnProperty issue – unless I'm wrong & there is magic to prevent the prototype chain being consulted.

> Source/JavaScriptCore/builtins/Array.prototype.js:63
> +    if (typeof callback !== "function")

OOI, I think there is a change in behavior here – I think the code currently checks whether the object is callable (as specified), whereas this is checking for functions. This means that callable non-functions will behave differently in these functions than in the previous implementation (and in host functions still written as native code) – e.g rexeps, callable API objects.

Probably not a big issue, but we should probably resolve this at some point.

> Source/JavaScriptCore/builtins/Array.prototype.js:96
> +            result[nextIndex++] = current;

This should be defineOwnProperty, if the prototype chain contains numeric setters or read only properties I think this will do the wrong thing?

> Source/JavaScriptCore/builtins/Array.prototype.js:122
> +        result[i] = callback.@call(thisArg, array[i], i, array)

This should be defineOwnProperty, if the prototype chain contains numeric setters or read only properties I think this will do the wrong thing?

[Oliver Hunt]

(In reply to comment #5)
> (From update of attachment 224141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224141&action=review
> 
> r- due to defineOwnProperty issue – unless I'm wrong & there is magic to prevent the prototype chain being consulted.
> 
> > Source/JavaScriptCore/builtins/Array.prototype.js:63
> > +    if (typeof callback !== "function")
> 
> OOI, I think there is a change in behavior here – I think the code currently checks whether the object is callable (as specified), whereas this is checking for functions. This means that callable non-functions will behave differently in these functions than in the previous implementation (and in host functions still written as native code) – e.g rexeps, callable API objects.

Hmmm, regexp, etc already match spec in this regard.  API objects also _should_ report correctly at this point.

> 
> Probably not a big issue, but we should probably resolve this at some point.
> 
> > Source/JavaScriptCore/builtins/Array.prototype.js:96
> > +            result[nextIndex++] = current;
> 
> This should be defineOwnProperty, if the prototype chain contains numeric setters or read only properties I think this will do the wrong thing?

Happily the code generator builtins work on the presumption of badness @Object() is the original constructor, all put_by_id and put_by_val are direct 

> 
> > Source/JavaScriptCore/builtins/Array.prototype.js:122
> > +        result[i] = callback.@call(thisArg, array[i], i, array)
> 
> This should be defineOwnProperty, if the prototype chain contains numeric setters or read only properties I think this will do the wrong thing?

as above.

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 224141 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=224141&action=review
> > 
> > r- due to defineOwnProperty issue – unless I'm wrong & there is magic to prevent the prototype chain being consulted.
> > 
> > > Source/JavaScriptCore/builtins/Array.prototype.js:63
> > > +    if (typeof callback !== "function")
> > 
> > OOI, I think there is a change in behavior here – I think the code currently checks whether the object is callable (as specified), whereas this is checking for functions. This means that callable non-functions will behave differently in these functions than in the previous implementation (and in host functions still written as native code) – e.g rexeps, callable API objects.
> 
> Hmmm, regexp, etc already match spec in this regard.  API objects also _should_ report correctly at this point.
> 

Just checked code - typeof (anything that can be called) is "function" the _only_ exception is those things that masquerade as undefined.  Because reasons.

> > 
> > Probably not a big issue, but we should probably resolve this at some point.

So not an issue at all anymore :)

[Oliver Hunt]

Comment on attachment 224141 [details]
Patch

Re-nominating for review.  Need to update some test expectations

[Gavin Barraclough]

Comment on attachment 224141 [details]
Patch

Oh, fair enough!

[Build Bot]

Comment on attachment 224141 [details]
Patch

Attachment 224141 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5964004941889536

New failing tests:
inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html

[Build Bot]

Created attachment 224182 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 224141 [details]
Patch

Attachment 224141 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5425206394880000

New failing tests:
inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html

[Build Bot]

Created attachment 224189 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Oliver Hunt]

Committed r164139: <http://trac.webkit.org/changeset/164139>