Bug 146275 - Optimize Array.join and Array.reverse for high speed array types
Summary: Optimize Array.join and Array.reverse for high speed array types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-23 23:41 PDT by Darin Adler
Modified: 2015-06-25 10:18 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.70 KB, patch)
2015-06-23 23:51 PDT, Darin Adler
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2015-06-23 23:41:46 PDT
Optimize Array.join and Array.reverse for high speed array types
Comment 1 Darin Adler 2015-06-23 23:51:54 PDT
Created attachment 255474 [details]
Patch
Comment 2 Darin Adler 2015-06-24 14:35:08 PDT
Any reviewer interested?
Comment 3 Mark Lam 2015-06-24 15:23:44 PDT
Comment on attachment 255474 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:9
> +        test from the Peacekeeper benchark.

typo: benchark

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:394
> +    JSString* separatorOnHeap;

This can be moved into the else clause below.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:422
> +                joiner.append(*exec, value);

Don't you have to check for a possible exception after this due to the joiner append()?  The contiguous indexing type can have arbitrary objects that will run toString() and therefore potentially throw an exception.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:471
> +                joiner.append(*exec, value);

Ditto.  Need exception check?
Comment 4 Darin Adler 2015-06-24 15:29:46 PDT
Comment on attachment 255474 [details]
Patch

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

Might need to add a bit of test coverage for the issues you spotted. Thanks for the review, Mark.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:394
>> +    JSString* separatorOnHeap;
> 
> This can be moved into the else clause below.

I don’t think it can. If it falls out of scope, then the separator, which could be a string computed by a toString function with no other references to it, might be garbage collected.

However, even if in scope, if it’s not used later I don’t know what guarantees the value will remain on the stack so it won’t be garbage collected. Maybe I need to do something else to make sure this works.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:422
>> +                joiner.append(*exec, value);
> 
> Don't you have to check for a possible exception after this due to the joiner append()?  The contiguous indexing type can have arbitrary objects that will run toString() and therefore potentially throw an exception.

Good point. I probably do. Let me think about it more.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:471
>> +                joiner.append(*exec, value);
> 
> Ditto.  Need exception check?

Will consider.
Comment 5 Andreas Kling 2015-06-24 15:39:28 PDT
(In reply to comment #4)
> >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:422
> >> +                joiner.append(*exec, value);
> > 
> > Don't you have to check for a possible exception after this due to the joiner append()?  The contiguous indexing type can have arbitrary objects that will run toString() and therefore potentially throw an exception.
> 
> Good point. I probably do. Let me think about it more.

Glorious refactoring idea: Enforce exception checking by encoding throwiness in signatures somehow, e.g like we do in WebCore with ExceptionCode& argument.
Comment 6 Darin Adler 2015-06-24 21:33:14 PDT
Committed r185942: <http://trac.webkit.org/changeset/185942>
Comment 7 Chris Dumez 2015-06-24 21:43:00 PDT
Looks like this change is causing build failures on a lot of bots:
https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/builds/16483/steps/compile-webkit/logs/stdio
Comment 8 Chris Dumez 2015-06-24 21:47:09 PDT
(In reply to comment #7)
> Looks like this change is causing build failures on a lot of bots:
> https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/
> builds/16483/steps/compile-webkit/logs/stdio

We probably need to #include "StrongInlines.h". I'll try this shortly.
Comment 9 Chris Dumez 2015-06-24 21:53:28 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Looks like this change is causing build failures on a lot of bots:
> > https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/
> > builds/16483/steps/compile-webkit/logs/stdio
> 
> We probably need to #include "StrongInlines.h". I'll try this shortly.

Landed in <http://trac.webkit.org/changeset/185943>.
Comment 10 Darin Adler 2015-06-25 10:18:06 PDT
Thanks so much for the build fix, Chris!