[es6] Optimize String{Starts, Ends}With

[es6] Optimize String{Starts, Ends}With

Replace Math{Min,Max}
Direct string comparison

Compared to https://codereview.chromium.org/1321853006/
single character
 found at true
77
P found at false
70
က found at false
70

BUG=v8:4384
LOG=N

Committed: https://crrev.com/b7db5cd9c761de2f15112ce4d46c7ade8c749ac0
Cr-Commit-Position: refs/heads/master@{#30631}

[skomski, 2015-09-07 09:05:43]

karl@skomski.com changed reviewers:
+ jkummerow@chromium.org, littledan@chromium.org

[skomski, 2015-09-07 09:23:38]

PTAL

[Dan Ehrenberg, 2015-09-08 07:24:55]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-08 07:25:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324353002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324353002/20001

[Dan Ehrenberg, 2015-09-08 07:27:58]

Other than style, looks good to me.

https://codereview.chromium.org/1324353002/diff/20001/src/string.js
File src/string.js (right):

https://codereview.chromium.org/1324353002/diff/20001/src/string.js#newcode1041
src/string.js:1041: return true;
There's some code duplication between these two functions. Can you factor any of
it out? For example, lines 1035 through 1041 are identical to 998-1003. If it's
marked inline, then I would expect that it wouldn't change performance.

[commit-bot: I haz the power, 2015-09-08 07:51:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 07:51:18]

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2015-09-08 08:13:04]

https://codereview.chromium.org/1324353002/diff/20001/src/string.js
File src/string.js (right):

https://codereview.chromium.org/1324353002/diff/20001/src/string.js#newcode1041
src/string.js:1041: return true;
On 2015/09/08 07:27:58, Dan Ehrenberg wrote:
> There's some code duplication between these two functions. Can you factor any
of
> it out? For example, lines 1035 through 1041 are identical to 998-1003. If
it's
> marked inline, then I would expect that it wouldn't change performance.

How do you mark a JavaScript function inline? :-)

I'd leave this as it is.

[Dan Ehrenberg, 2015-09-08 08:15:19]

lgtm

[skomski, 2015-09-08 09:40:46]

Did a quick benchmark with positions and found out there is an deoptimization
pattern in some string functions:

  if (%_ArgumentsLength() > 1) {
    pos = %_Arguments(1);  // position
    pos = TO_INTEGER(pos);
  }

should be

  if (%_ArgumentsLength() > 1) {
    var arg = %_Arguments(1);  // position
    if (!IS_UNDEFINED(arg)) {
      pos = $toInteger(arg);
    }
  }

results into

String.startsWith
OLD  NEW
search: warmup index:undefined
2105 1021
search:  index:undefined
2136 1042
search:  index:0
1122 1179
search: 
 index:1
1117 1184
search: P index:undefined
2109 975
search: က index:undefined
2093 971
search: က index:100
1050 1119

A further optimization about 250 ms would be to directly specify the argument to
remove ArgumentsLength() and _Arguments() but then
there need to be some way to fake the length property for the ecma spec.
Updated in StartsWith. I will make a new review for all other affected
functions.

[Jakob Kummerow, 2015-09-08 09:46:10]

LGTM with nits.

Doing the !IS_UNDEFINED check at callsites is rather sad. It would be nice if
$toInteger took care of that without deopting; but changing that might be more
involved as it might have unexpected side effects.

https://codereview.chromium.org/1324353002/diff/40001/src/string.js
File src/string.js (right):

https://codereview.chromium.org/1324353002/diff/40001/src/string.js#newcode1002
src/string.js:1002: return false;
nit: {} for multi-line ifs

https://codereview.chromium.org/1324353002/diff/40001/src/string.js#newcode1040
src/string.js:1040: return false;
nit: {} for multi-line ifs

[skomski, 2015-09-08 09:54:56]

The CQ bit was checked by karl@skomski.com

[skomski, 2015-09-08 09:54:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org, jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1324353002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2015-09-08 09:55:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324353002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324353002/60001

[commit-bot: I haz the power, 2015-09-08 10:20:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-08 10:20:55]

Patchset 4 (id:??) landed as
https://crrev.com/b7db5cd9c761de2f15112ce4d46c7ade8c749ac0
Cr-Commit-Position: refs/heads/master@{#30631}

[Lasse Reichstein Nielsen, 2015-09-08 10:24:57]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2015-09-08 10:24:57]

https://codereview.chromium.org/1324353002/diff/60001/src/string.js
File src/string.js (right):

https://codereview.chromium.org/1324353002/diff/60001/src/string.js#newcode993
src/string.js:993: if (pos > s_len) pos = s_len;
DBC: Would it be useful to make this an "else" of the first test since both
can't be true?
And move the tests inside the second if above (no need to do them if pos is 0)?

https://codereview.chromium.org/1324353002/diff/60001/src/string.js#newcode1037
src/string.js:1037: }
The spec is really overspecifying here - if searchString is the empty string, a
lot of this work is unnecessary.

Maybe just::

   ... 
   pos = $toInteger(arg);
   if (pos > s_len) pos = s_len;
  }
 }
 var ss_len = ss.length;
 if (pos < ss_len) return ss_len == 0;