Bug 47506 – Optimize String.prototype.replace on v8-regexp

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

[Michael Saboff]

About 10% of v8-regexp time is spent in stringProtoFuncReplace() and supporting routines.

[Michael Saboff]

Created attachment 70481 [details]
Patch to reduce overhead.

This patch streamlines some of the processing after the regular expression has been applied to the string.  Replaced a null string (len ==0) with default constructors.  Other misc. string aiti issues.

[WebKit Review Bot]

Comment on attachment 70481 [details]
Patch to reduce overhead.

Rejecting patch 70481 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Oliver Hunt]

Comment on attachment 70481 [details]
Patch to reduce overhead.

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

Basic logic looks fine, but i don't really like the assignment to length inside those if conditions, so r-

> JavaScriptCore/runtime/StringPrototype.cpp:278
> +        if ((i < rangeCount) && (length = substringRanges[i].length)) {

This seems a little icky, i'd prefer it if we didn't assign to length inside this condition.

You could try just:
if (i < rangeCount) {
    int length = substringRanges[i].length;
    StringImpl::copyChars(buffer + bufferPos, source.characters() + substringRanges[i].position, length);
    bufferPos += length;
}

This allows the outer int length to be removed (so avoiding confusion over what length is being referred to), and i would hope a 0 length string copy would have negligible impact.

> JavaScriptCore/runtime/StringPrototype.cpp:282
> +        if ((i < separatorCount) && (length = separators[i].length())) {

ditto.

[Darin Adler]

(In reply to comment #3)
> i would hope a 0 length string copy would have negligible impact.

We could do more than hope; we could measure. The question is whether the extra branch to check for zero length pays for itself. This depends on: 1) how costly the copyChars with length 0 is, 2) how costly the branch is, and 3) how common a length of zero is.

[Michael Saboff]

Created attachment 70545 [details]
Updated patch

Changed the embedded assignments in the if statements to be independent and include declarations.

Concerning comment #4, the impact of the copying of zero length strings is measurable although I didn't break it out separately.  The StringImpl::copyChars() actually result in a function call (not inlined).  The zero length check before the call eliminates not only the call but a few other instructions as well.

[WebKit Commit Bot]

Comment on attachment 70545 [details]
Updated patch

Clearing flags on attachment: 70545

Committed r69590: <http://trac.webkit.org/changeset/69590>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #5)
> The StringImpl::copyChars() actually results in a function call (not inlined).

As a small related cleanup and performance task we may want to look into renaming copyChars, making the renamed copyCharacters an inline function, and also making it be a free function rather than a StringImpl static member function.

I suspect this would be better for almost every call site, it might allow us to remove this zero length check as well, and perhaps it would even make some benchmarks run faster.