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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 9 13:29:00 PDT 2019

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

Attachment 373745: Patch


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

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.

More information about the webkit-reviews mailing list