[webkit-reviews] review granted: [Bug 199636] Optimize join of large empty arrays : [Attachment 373745] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 199636: Optimize join of large empty arrays
https://bugs.webkit.org/show_bug.cgi?id=199636

Attachment 373745: Patch

https://bugs.webkit.org/attachment.cgi?id=373745&action=review




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 373745
  --> https://bugs.webkit.org/attachment.cgi?id=373745
Patch

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

I think one the changes in your patch is that you're creating a JSRopeString
instead of a regular JSString.	In this microbenchmark, the result string is
never consumed as a string.  As a result, we never incur the cost of resolving
the rope.  In contrast, the old default path that falls thru to the generalCase
would eagerly resolve the rope.  What this means is that in practice, you may
potentially be shifting the workload to a later time.  Just in case, please run
benchmarks to make sure there is no regression and include the benchmark
results in the ChangeLog.

r=me if EWS is green and there's no perf regression.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:521
> +	       JSString* operand = jsString(&vm, separator.toString());

jsString() allocates and can throw an exception.  So, you'll need a
RETURN_IF_EXCEPTION(scope, { }) here.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:522
>> +		unsigned count = length - 1;
> 
> Do we need `length == 0` handling?

This is already handled in "case 0:" above.