Closed Bug 601296 Opened 14 years ago Closed 14 years ago

speedup FindReplaceLength

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files, 4 obsolete files)

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.
Attached patch inline js_ValueToString case (obsolete) — Splinter Review
Inline string case for js_ValueToString.
Attachment #480324 - Flags: review?(cdleary)
Attached patch don't copy RegExpStatics (obsolete) — Splinter Review
Attachment #480325 - Flags: review?(cdleary)
Attachment #480324 - Attachment description: patch → inline js_ValueToString case
Attached patch inline JS_GetEmptyStringValue (obsolete) — Splinter Review
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. :-)
Attachment #480324 - Attachment is obsolete: true
Attachment #480741 - Flags: review?(cdleary)
Attachment #480324 - Flags: review?(cdleary)
Attachment #480325 - Attachment is obsolete: true
Attachment #481387 - Flags: review?(cdleary)
Attachment #480325 - Flags: review?(cdleary)
Attachment #480741 - Attachment is obsolete: true
Attachment #481388 - Flags: review?(cdleary)
Attachment #480741 - Flags: review?(cdleary)
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+
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/0d4a3205dd6f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.