Optimize Array.join and Array.reverse for high speed array types
Created attachment 255474 [details] Patch
Any reviewer interested?
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 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.
(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.
Committed r185942: <http://trac.webkit.org/changeset/185942>
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
(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.
(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>.
Thanks so much for the build fix, Chris!