Closed
Bug 1383646
Opened 7 years ago
Closed 7 years ago
Maybe optimize when String.prototype.indexOf is called with string==searchString ?
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: anba, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.64 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Speedometer (all frameworks run once from InteractiveRunner.html) Num Type Disp Enb Address What 9 breakpoint keep y 0x00007fffe8d2eba8 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) at /home/andre/hg/mozilla-inbound/js/src/jsstr.cpp:2171 stop only if str.ptr==searchStr.ptr breakpoint already hit 10095 times 10 breakpoint keep y 0x00007fffe8d2eba8 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) at /home/andre/hg/mozilla-inbound/js/src/jsstr.cpp:2171 stop only if str.ptr->d.u1.length<searchStr.ptr->d.u1.length breakpoint already hit 11131 times 11 breakpoint keep y 0x00007fffe8d2eba8 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) at /home/andre/hg/mozilla-inbound/js/src/jsstr.cpp:2171 breakpoint already hit 79549 times Only AngularJS: Num Type Disp Enb Address What 6 breakpoint keep y 0x00007fffe8d2eba8 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) at /home/andre/hg/mozilla-inbound/js/src/jsstr.cpp:2171 stop only if str.ptr==searchStr.ptr breakpoint already hit 10088 times 7 breakpoint keep y 0x00007fffe8d2eba8 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) at /home/andre/hg/mozilla-inbound/js/src/jsstr.cpp:2171 stop only if str.ptr->d.u1.length<searchStr.ptr->d.u1.length breakpoint already hit 10182 times 8 breakpoint keep y 0x00007fffe8d2eba8 in js::str_indexOf(JSContext*, unsigned int, JS::Value*) at /home/andre/hg/mozilla-inbound/js/src/jsstr.cpp:2171 breakpoint already hit 22691 times This is kind of stupid to optimize, but AngularJS [1] calls |"false".indexOf("false")| (and |"true".indexOf("false")|) quite often and str_indexOf is the fifth most called builtin per bug 1365361. [1] https://github.com/WebKit/webkit/blob/80b1e93d42aac34fddd652bb6f7749d26a265d36/PerformanceTests/Speedometer/resources/todomvc/architecture-examples/angularjs/node_modules/angular/angular.js#L18303-L18305
Assignee | ||
Comment 1•7 years ago
|
||
Assignee: nobody → evilpies
Attachment #8890822 -
Flags: review?(andrebargull)
Reporter | ||
Comment 2•7 years ago
|
||
Comment on attachment 8890822 [details] [diff] [review] Optimize (last)indexOf when str equals searchString Review of attachment 8890822 [details] [diff] [review]: ----------------------------------------------------------------- I was wondering if it makes sense to replace http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/js/src/jsstr.cpp#2167-2173 with: // Handle the common case first: the search string is strictly smaller // than the text string. We don't compare against |pos + textLen| to keep // this case fast by avoiding the extra addition. if (searchStr->length() < textLen) { // Steps 10 and 11 ... } if (searchStr->length() == textLen && start == 0) { ... Call EqualString(). } ... Directly return -1. But then again I didn't want to penalise proper usage of indexOf() with too many extra checks for the two special cases from AngularJS. So, let's just add the simple pointer comparison |str == searchStr| for now, hopefully that's sufficient for bug 1365361. :-) ::: js/src/jsstr.cpp @@ +2163,5 @@ > > // Step 9 > uint32_t start = Min(Max(pos, 0U), textLen); > > + if (str == searchStr) { We should add a comment to explain why we take exactly this short-cut. And not, let's say |if (textLen == searchStr->length()) ...|. @@ +2252,5 @@ > } > } > } > > + if (str == searchStr) { Hmm, I'm not sure we should also add the fast-path to lastIndexOf, unless we have evidence it also occurs in workloads we care about.
Attachment #8890822 -
Flags: review?(andrebargull) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Thanks for reviewing. I think the simple pointer comparison is cheap enough that we don't need to worry. For changes like this I like to keep a bit of symmetry and not doing this for lastIndexOf just seems weird.
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8790aae878 Optimize x.indexOf(x) for AngularJS. r=anba
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #3) > Thanks for reviewing. I think the simple pointer comparison is cheap enough > that we don't need to worry. For changes like this I like to keep a bit of > symmetry and not doing this for lastIndexOf just seems weird. Okay, understood! :-)
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5f8790aae878
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•