Run string replace regexp with function in C++ code loop.

Run string replace regexp with function in C++ code loop.
Reuses the result array to save on allocation.
Matches Safari's behavior.

[Lasse Reichstein, 2010-03-23 13:37:25]

Largish review.

[Lasse Reichstein, 2010-03-24 13:37:20]

Reallocate reviewer.

[fschneider, 2010-03-25 10:26:06]

LGTM with a couple of suggestions and style comments.

http://codereview.chromium.org/1109010/diff/3001/4001
File src/jsregexp.h (right):

http://codereview.chromium.org/1109010/diff/3001/4001#newcode145
src/jsregexp.h:145: static bool IsAtom(FixedArray* array) {
Is this function ever called?

http://codereview.chromium.org/1109010/diff/3001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1619
src/runtime.cc:1619: inline Handle<FixedArray> array() {
I don't know enough how C++ compilers treat the inline keyword, but I'm not sure
if we need inline here if the function is defined at the declaration.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1650
src/runtime.cc:1650: static const int kStringBuilderConcatHelperLengthBits = 11;
No need for static here. It is the default for C++ top-level const declarations.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1659
src/runtime.cc:1659: typedef BitField<int, 0, 11> StringBuilderSubstringLength;
Would be better to use the defined constants here. Although the line may get too
long then...

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1763
src/runtime.cc:1763: 
Extra empty line.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode2953
src/runtime.cc:2953: static const int kMaxBuilderEntriesPerRegExpMatch = 5;
No need for static here either.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode2988
src/runtime.cc:2988: if (new_pos >= 0) {
More compact if you reverse the condition:

if (new_pos < 0) break;
...

http://codereview.chromium.org/1109010/diff/3001/4003#newcode2989
src/runtime.cc:2989: // A match.
Complete sentence for whole-line comments.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3013
src/runtime.cc:3013: uc16 pattern_char = pattern->Get(0);
ASSERT that pattern is a uc16-string?

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3021
src/runtime.cc:3021: if (pattern_char > String::kMaxAsciiCharCode) {
No need for multi-line if.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3069
src/runtime.cc:3069: case SEARCH_SHORT: {
No need for { } block?

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3078
src/runtime.cc:3078: if (new_pos >= 0) {
Reverse condition here too?

if (new_pos < 0) break;
...

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3093
src/runtime.cc:3093: case SEARCH_LONG: {
No need for { } block?

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3102
src/runtime.cc:3102: if (new_pos >= 0) {
And here?

if (new_pos < 0) break;
...

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3312
src/runtime.cc:3312: Vector<int> tmp = prev_register_vector;
Could you have one stack-allocated instance of the tmp-Vector and reuse it
instead of constructing a new one for each swap?

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3384
src/runtime.cc:3384: if (pattern_length == 1) {
Or just:

if (pattern->length() == 1 &&
    SearchCharMultiple(...)) {
  return *builder.ToJSArray(result_array);
}

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3419
src/runtime.cc:3419: if (result == RegExpImpl::RE_SUCCESS) return
*builder.ToJSArray(result_array);
The test for the result seems duplicated. You could write:

RegExpImpl::IrregexpResult result;
if (capture_count == 0) {
  result = SearchRegExpNoCaptureMultiple(...);
} else {
  result = SearchRegExpMultiple(...)'
}
if (result == RegExpImpl::RE_SUCCESS) return ...
if (result == RegExpImpl::RE_FAILURE) return ...
ASSERT_EQ(result, RegExpImpl::RE_EXCEPTION);
return Failure::Exception();

http://codereview.chromium.org/1109010/diff/3001/4005
File src/string.js (right):

http://codereview.chromium.org/1109010/diff/3001/4005#newcode434
src/string.js:434: regexp.lastIndex = 0; // why this?
Clarify this comment.

[Lasse Reichstein, 2010-03-26 11:18:26]

http://codereview.chromium.org/1109010/diff/3001/4001
File src/jsregexp.h (right):

http://codereview.chromium.org/1109010/diff/3001/4001#newcode145
src/jsregexp.h:145: static bool IsAtom(FixedArray* array) {
I don't think so. I switched to using JSRegExp::IsAtom instead. 
Will remove.

http://codereview.chromium.org/1109010/diff/3001/4003
File src/runtime.cc (right):

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1619
src/runtime.cc:1619: inline Handle<FixedArray> array() {
I think you are right. Inlining is default in this case.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1650
src/runtime.cc:1650: static const int kStringBuilderConcatHelperLengthBits = 11;
Fixed.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode1659
src/runtime.cc:1659: typedef BitField<int, 0, 11> StringBuilderSubstringLength;
Very long, but changed anyway.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode2953
src/runtime.cc:2953: static const int kMaxBuilderEntriesPerRegExpMatch = 5;
Removed.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode2988
src/runtime.cc:2988: if (new_pos >= 0) {
I want to have the "fast case" as the first branch of the if.
I guess the compiler would generate the exact same code in both cases, though. 
I'll keep this for now.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3013
src/runtime.cc:3013: uc16 pattern_char = pattern->Get(0);
Not necessary. Get is generic and works on both kinds of strings, and always
return uc16.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3021
src/runtime.cc:3021: if (pattern_char > String::kMaxAsciiCharCode) {
How not?

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3069
src/runtime.cc:3069: case SEARCH_SHORT: {
Done.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3078
src/runtime.cc:3078: if (new_pos >= 0) {
Same argument as last time, I want the "then" branch to be the fast case (the
one that keeps looping).

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3093
src/runtime.cc:3093: case SEARCH_LONG: {
Done.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3102
src/runtime.cc:3102: if (new_pos >= 0) {
Also retained.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3312
src/runtime.cc:3312: Vector<int> tmp = prev_register_vector;
Vector is a two-value struct. Allocating a single stack-vector won't be faster
than allocating a new each time, it'll be at the same place on the stack anyway.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3384
src/runtime.cc:3384: if (pattern_length == 1) {
That wouldn't return null if SearchCharMultiple returns false.

http://codereview.chromium.org/1109010/diff/3001/4003#newcode3419
src/runtime.cc:3419: if (result == RegExpImpl::RE_SUCCESS) return
*builder.ToJSArray(result_array);
On 2010/03/25 10:26:06, fschneider wrote:
> The test for the result seems duplicated. You could write:
> 
> RegExpImpl::IrregexpResult result;
> if (capture_count == 0) {
>   result = SearchRegExpNoCaptureMultiple(...);
> } else {
>   result = SearchRegExpMultiple(...)'
> }
> if (result == RegExpImpl::RE_SUCCESS) return ...
> if (result == RegExpImpl::RE_FAILURE) return ...
> ASSERT_EQ(result, RegExpImpl::RE_EXCEPTION);
> return Failure::Exception();

Done.

http://codereview.chromium.org/1109010/diff/3001/4005
File src/string.js (right):

http://codereview.chromium.org/1109010/diff/3001/4005#newcode434
src/string.js:434: regexp.lastIndex = 0; // why this?
Removed. There is no doubt why if one reads the specification of
RegExp.prototype.replace :)