Closed
Bug 601296
Opened 14 years ago
Closed 14 years ago
speedup FindReplaceLength
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(3 files, 4 obsolete files)
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Inline string case for js_ValueToString.
Attachment #480324 -
Flags: review?(cdleary)
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #480325 -
Flags: review?(cdleary)
Assignee | ||
Updated•14 years ago
|
Attachment #480324 -
Attachment description: patch → inline js_ValueToString case
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #480328 -
Flags: review?(cdleary)
Comment 4•14 years ago
|
||
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•14 years ago
|
||
Attachment #480324 -
Attachment is obsolete: true
Attachment #480741 -
Flags: review?(cdleary)
Attachment #480324 -
Flags: review?(cdleary)
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #480325 -
Attachment is obsolete: true
Attachment #481387 -
Flags: review?(cdleary)
Attachment #480325 -
Flags: review?(cdleary)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #480741 -
Attachment is obsolete: true
Attachment #481388 -
Flags: review?(cdleary)
Attachment #480741 -
Flags: review?(cdleary)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #480328 -
Attachment is obsolete: true
Attachment #481389 -
Flags: review?(cdleary)
Attachment #480328 -
Flags: review?(cdleary)
Updated•14 years ago
|
Attachment #481388 -
Flags: review?(cdleary) → review+
Updated•14 years ago
|
Attachment #481389 -
Flags: review?(cdleary) → review+
Comment 9•14 years ago
|
||
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•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey
Comment 11•14 years ago
|
||
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.
Description
•