Bug 128788 - Implement a few more Array prototype functions in JS
Summary: Implement a few more Array prototype functions in JS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-13 18:06 PST by Oliver Hunt
Modified: 2014-02-14 15:24 PST (History)
2 users (show)

See Also:


Attachments
Patch (42.16 KB, patch)
2014-02-13 18:15 PST, Oliver Hunt
barraclough: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (499.61 KB, application/zip)
2014-02-13 20:46 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (498.92 KB, application/zip)
2014-02-14 02:11 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (549.49 KB, application/zip)
2014-02-14 02:59 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2014-02-13 18:06:31 PST
Implement a few more Array prototype functions in JS
Comment 1 Oliver Hunt 2014-02-13 18:15:07 PST
Created attachment 224141 [details]
Patch
Comment 2 Build Bot 2014-02-13 20:46:27 PST
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
Comment 3 Build Bot 2014-02-13 20:46:28 PST
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
Comment 4 Ryosuke Niwa 2014-02-13 21:26:38 PST
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
Comment 5 Gavin Barraclough 2014-02-13 22:31:53 PST
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?
Comment 6 Oliver Hunt 2014-02-13 22:53:27 PST
(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.
Comment 7 Oliver Hunt 2014-02-13 22:56:08 PST
(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 :)
Comment 8 Oliver Hunt 2014-02-13 22:57:03 PST
Comment on attachment 224141 [details]
Patch

Re-nominating for review.  Need to update some test expectations
Comment 9 Gavin Barraclough 2014-02-13 23:48:54 PST
Comment on attachment 224141 [details]
Patch

Oh, fair enough!
Comment 10 Build Bot 2014-02-14 02:11:51 PST
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
Comment 11 Build Bot 2014-02-14 02:11:53 PST
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
Comment 12 Build Bot 2014-02-14 02:59:41 PST
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
Comment 13 Build Bot 2014-02-14 02:59:43 PST
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
Comment 14 Oliver Hunt 2014-02-14 15:24:38 PST
Committed r164139: <http://trac.webkit.org/changeset/164139>