1383646 - Maybe optimize when String.prototype.indexOf is called with string==searchString ?

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

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

Comment 1

[Tom S [:evilpie]]

Attachment: Optimize (last)indexOf when str equals searchString

Comment 2

[André Bargull [:anba]]

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.

Comment 3

[Tom S [:evilpie]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8790aae878
Optimize x.indexOf(x) for AngularJS. r=anba

Comment 5

[André Bargull [: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! :-)

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f8790aae878