Bug 146275 – Optimize Array.join and Array.reverse for high speed array types

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

Optimize Array.join and Array.reverse for high speed array types

[Darin Adler]

Created attachment 255474 [details]
Patch

[Darin Adler]

Any reviewer interested?

[Mark Lam]

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?

[Darin Adler]

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.

[Andreas Kling]

(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.

[Darin Adler]

Committed r185942: <http://trac.webkit.org/changeset/185942>

[Chris Dumez]

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

[Chris Dumez]

(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.

[Chris Dumez]

(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>.

[Darin Adler]

Thanks so much for the build fix, Chris!