Hmmm, I forgot about making the native method robust. How important
is this? I guess it's just good for quality over all. If you call
%StringIndexOf("a", "", 0) now, it will be bad, since both the KMP and
the brute force code expect a non-zero pattern. I will try to think
of a way to handle this (the result doesn't need to be correct though,
right?)...
Maybe I should just understand the regression better too...
On Sat, Sep 20, 2008 at 4:06 PM, <[EMAIL PROTECTED]> wrote:
> Reviewers: Mads Ager, Erik Corry,
>
> Message:
> This "fixes" our 10% regression on a string benchmark. I was only able
> to reproduce the regression on a P4 system. I'd like to know better
> what was going on, but there was too many things that changed. I diffed
> the generated code, and it looks like it comes down to either 1) stack
> size increase, but I don't think this was it, 2) register allocation,
> there were a few spots where the compiler used the stack instead of a
> register, 3) code size. I would say it's mostly 3, which is strange.
> It didn't look like any of the branching or anything else changed. I
> can dig into it further if needed.
>
> I think moving this code out makes sense anyway, and we get better
> numbers that before on this test, which is a bit unexpected. In
> general, I'd like to understand the P4 performance issues better, if
> anyone has insight, I'd love to know.
>
> Description:
> Move the pattern.length == 0 check from the runtime to JS wrapper code.
> We already have the length and index in the wrapper, so this makes
> sense, and will avoid the runtime in this case. Cleaned up the naming
> to be consistent between the runtime and JS code.
>
> Please review this at http://codereview.chromium.org/3181
>
> Affected files:
> M src/runtime.cc
> M src/string.js
>
>
> Index: src/runtime.cc
> diff --git a/src/runtime.cc b/src/runtime.cc
> index
> 44bfb3bfcf633424146ffb65df5129bb051cec47..73fe260793784d1d13a0d924289c8b14b8889f3e
> 100644
> --- a/src/runtime.cc
> +++ b/src/runtime.cc
> @@ -909,7 +909,8 @@ static Object* Runtime_StringIndexOf(Arguments args) {
>
> CONVERT_CHECKED(String, sub, args[0]);
> CONVERT_CHECKED(String, pat, args[1]);
> - Object* index = args[2];
> + uint32_t start_index;
> + if (!Array::IndexFromObject(args[2], &start_index)) return
> Smi::FromInt(-1);
>
> sub->TryFlatten();
> pat->TryFlatten();
> @@ -917,10 +918,6 @@ static Object* Runtime_StringIndexOf(Arguments args) {
> int subject_length = sub->length();
> int pattern_length = pat->length();
>
> - uint32_t start_index;
> - if (!Array::IndexFromObject(index, &start_index)) return
> Smi::FromInt(-1);
> - if (pattern_length == 0) return Smi::FromInt(start_index);
> -
> // Searching for one specific character is common. For one
> // character patterns the KMP algorithm is guaranteed to slow down
> // the search, so we just run through the subject string.
> Index: src/string.js
> diff --git a/src/string.js b/src/string.js
> index
> d8105fdfc0b88ffaa02a0e8ec110559a4c3b79a4..272eef0b85c1055b1c527b47948a33c9868a308e
> 100644
> --- a/src/string.js
> +++ b/src/string.js
> @@ -331,18 +331,20 @@ function ApplyReplacementFunction(replace, captures,
> subject) {
>
> // ECMA-262 section 15.5.4.7
> %AddProperty($String.prototype, "indexOf", function(searchString /*
> position */) { // length == 1
> - var str = ToString(this);
> - var str_len = str.length;
> - var searchStr = ToString(searchString);
> + var subject_str = ToString(this);
> + var pattern_str = ToString(searchString);
> + var subject_str_len = subject_str.length;
> + var pattern_str_len = pattern_str.length;
> var index = 0;
> if (%_ArgumentsLength() > 1) {
> var arg1 = %_Arguments(1); // position
> index = TO_INTEGER(arg1);
> }
> if (index < 0) index = 0;
> - if (index > str_len) index = str_len;
> - if (searchStr.length + index > str_len) return -1;
> - return %StringIndexOf(str, searchStr, index);
> + if (index > subject_str_len) index = subject_str_len;
> + if (pattern_str_len == 0) return index;
> + if (pattern_str_len + index > subject_str_len) return -1;
> + return %StringIndexOf(subject_str, pattern_str, index);
> }, DONT_ENUM);
>
>
>
>
>
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
