speedup FindReplaceLength

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Callgrind shows a fair amount of time in str_replace under ReplaceCallback.  The attached patches remove about ~10M instructions (3.3%) for a 2-3ms speedup (6%) (-jm) on unpack-code.

The biggest improvement was to avoid copying RegExpStatics back and forth by doing copy-on-write.  Smaller improvements came from inlining the string case for js_ValueToString and using cx->runtime->emptyString instead of JS_GetEmptyStringValue.
(Assignee)

Comment 1

7 years ago
Created attachment 480324 [details] [diff] [review]
inline js_ValueToString case

Inline string case for js_ValueToString.
Attachment #480324 - Flags: review?(cdleary)
(Assignee)

Comment 2

7 years ago
Created attachment 480325 [details] [diff] [review]
don't copy RegExpStatics
Attachment #480325 - Flags: review?(cdleary)
(Assignee)

Updated

7 years ago
Attachment #480324 - Attachment description: patch → inline js_ValueToString case
(Assignee)

Comment 3

7 years ago
Created attachment 480328 [details] [diff] [review]
inline JS_GetEmptyStringValue
Attachment #480328 - Flags: review?(cdleary)
Wow cool, when I last implemented COW for the statics it didn't have a visible speedup -- maybe this is a sign we're Doing It Right. :-)
(Assignee)

Comment 5

7 years ago
Created attachment 480741 [details] [diff] [review]
inline js_ValueToString for string case (fix thinko)
Attachment #480324 - Attachment is obsolete: true
Attachment #480741 - Flags: review?(cdleary)
Attachment #480324 - Flags: review?(cdleary)
(Assignee)

Comment 6

7 years ago
Created attachment 481387 [details] [diff] [review]
don't copy RegExpStatics (rebased)
Attachment #480325 - Attachment is obsolete: true
Attachment #481387 - Flags: review?(cdleary)
Attachment #480325 - Flags: review?(cdleary)
(Assignee)

Comment 7

7 years ago
Created attachment 481388 [details] [diff] [review]
inline js_ValueToString for string case (rebased)
Attachment #480741 - Attachment is obsolete: true
Attachment #481388 - Flags: review?(cdleary)
Attachment #480741 - Flags: review?(cdleary)
(Assignee)

Comment 8

7 years ago
Created attachment 481389 [details] [diff] [review]
inline JS_GetEmptyStringValue (rebased)
Attachment #480328 - Attachment is obsolete: true
Attachment #481389 - Flags: review?(cdleary)
Attachment #480328 - Flags: review?(cdleary)
Attachment #481388 - Flags: review?(cdleary) → review+
Attachment #481389 - Flags: review?(cdleary) → review+
Comment on attachment 481387 [details] [diff] [review]
don't copy RegExpStatics (rebased)

Looks good!

We have to traverse to the bufferLink every time we're about to write, even if we've already copied to it -- we could flip it around and use a (preserved) "hasBeenCopiedToBufferLink" member instead of our current "hasTakenDataFromOriginal" approach if we find that traversal is hot in |aboutToWrite|. Maybe a comment about this is worthwhile? Not sure, might just end up dominated by other regexpy things.

I was thinking it might be nice to have a guard type returned from |aboutToWrite| to get at mutable matchPairs, but that seems a bit much for the limited exposure to js::RegExp that we have at the moment. A comment on |aboutToWrite| about the circumstances under which you have to call it seems like it'd be plenty!

Good catch reporting out of memory, res used to be tied to a cx. Also on the missing OOM check for clone in PreserveRES. Blargh.

It would also be cool to know we have at least one test that checks that this is performed properly, of this kind of form:

'abcdef'.replace(/a(\w+)c/, function() {
    assertEq(RegExp.lastMatch, 'abc');
    '123456'.replace(/1(\d+)3/, function() {
        assertEq(RegExp.lastMatch, '123');
    });
    assertEq(RegExp.lastMatch, '123');
});
assertEq(RegExp.lastMatch, 'abc');

More minor nits:
- Would be nice to have a comment in |copyTo| that points out the space is reserved for a bufferLink in |save|.
- Don't need explicit anymore on the zero-argument constructor (used to take a JSContext, but no more!).
Attachment #481387 - Flags: review?(cdleary) → review+
(Assignee)

Comment 10

7 years ago
Thanks Chris!

http://hg.mozilla.org/tracemonkey/rev/0d4a3205dd6f
http://hg.mozilla.org/tracemonkey/rev/fcd0d0dbf928
http://hg.mozilla.org/tracemonkey/rev/ced6d4651b6a
(Assignee)

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey

Comment 11

7 years ago
http://hg.mozilla.org/mozilla-central/rev/0d4a3205dd6f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.