Closed Bug 842192 Opened 9 years ago Closed 9 years ago

Self-host Array.map

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: till, Assigned: till)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached patch v1Splinter Review
While the self-hosted version of Array.map isn't many times as fast as the native one, as was the case for Array.forEach, a simple test-case using the mapping function `function(item, idx, list) {return item * idx}` on a map with 10000000 entries went from 1110ms to 645ms for me.

Also: 2 files changed, 51 insertions(+), 87 deletions(-)
Attachment #714986 - Flags: review?(jorendorff)
Attached file Simple benchmark
Simple benchmark, showing a speedup of about 2x.

Changing the UnsafeSetElement in Step c.iii to `A[i] = mappedValue` improves the speedup to about 7x.
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Attachment #716519 - Flags: feedback?(nmatsakis)
Comment on attachment 716519 [details]
Simple benchmark

After talking to nmatsakis on IRC, I updated to current tip.

With that, the self-hosted Array.map is now ~6.5x as fast as the native version: from 1110ms to 179ms.
Attachment #716519 - Flags: feedback?(nmatsakis)
Comment on attachment 714986 [details] [diff] [review]
v1

Review of attachment 714986 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Array.js
@@ +384,5 @@
> +}
> +
> +function ArrayStaticMap(list, fun/*, receiver */) {
> +    if (arguments.length < 2)
> +        ThrowError(JSMSG_MISSING_FUN_ARG, 0, 'Array.map');

It would be consistent here to check whether fun is callable, as in ArrayStaticForEach and friends (in order to produce a better error message).

@@ +385,5 @@
> +
> +function ArrayStaticMap(list, fun/*, receiver */) {
> +    if (arguments.length < 2)
> +        ThrowError(JSMSG_MISSING_FUN_ARG, 0, 'Array.map');
> +    var receiver = arguments.length > 1 ? arguments[1] : void 0;

2, not 1, right?

@@ +386,5 @@
> +function ArrayStaticMap(list, fun/*, receiver */) {
> +    if (arguments.length < 2)
> +        ThrowError(JSMSG_MISSING_FUN_ARG, 0, 'Array.map');
> +    var receiver = arguments.length > 1 ? arguments[1] : void 0;
> +    callFunction(ArrayMap, list, fun, receiver);

Need a return statement.

::: js/src/jsarray.cpp
@@ +2491,5 @@
>  
>           {"lastIndexOf",        {NULL, NULL},       1,0, "ArrayLastIndexOf"},
>           {"indexOf",            {NULL, NULL},       1,0, "ArrayIndexOf"},
>           {"forEach",            {NULL, NULL},       1,0, "ArrayForEach"},
> +         {"map",                {NULL, NULL},       1,0, "ArrayMap"},

We need an entry for ArrayStaticMap in array_static_methods.
Attachment #714986 - Flags: review?(jorendorff) → review+
Pushed with comments addressed and some naming fixes
https://hg.mozilla.org/integration/mozilla-inbound/rev/788c24847525
Try run for 0a987a59e6b2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0a987a59e6b2
Results (out of 40 total builds):
    success: 38
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tschneidereit@gmail.com-0a987a59e6b2
https://hg.mozilla.org/mozilla-central/rev/788c24847525
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.