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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: anba, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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: nobody → evilpies
Attachment #8890822 - Flags: review?(andrebargull)
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+
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
(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! :-)
https://hg.mozilla.org/mozilla-central/rev/5f8790aae878
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.