Optimize non-ASCII string splitting with single-character search pattern

Optimize non-ASCII string splitting with single-character search pattern

Committed: https://code.google.com/p/v8/source/detail?r=13119

[bnoordhuis1, 2012-11-23 22:38:59]

string: optimize non-ASCII string splitting

Optimize String.split() for UCS-2 strings where the search pattern is a single
ASCII or UCS-2 character.
    
Gives a 15% performance boost on x64 with the below micro-benchmark:
    
  var s = "Zum iech jeitzt wa, der stolz fergiess Dauschen op. " +
          "Dat rout Mier Faarwen no. Halm fond d'Blumme do hie, " +
          "Kënnt dénen derfir dat ke. Wuel main rem vu, hu main " +
          "zwëschen gei. Un lait schaddreg hie, gin hire drun " +
          "Hémecht en, hun hire dämpen da.";
  for (var i = 0; i < 1e7; ++i) s.split(" ");

[Yang, 2012-11-26 08:45:30]

Thanks for this patch! I have some drive-by comments.

https://codereview.chromium.org/11299163/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11299163/diff/1/src/objects.h#newcode7195
src/objects.h:7195: if (state_ == TWO_BYTE) return buffer_.length() / 2;
you could do an
if (state_ == ASCII) {
  ...
} else {
  ASSERT(state == TWO_BYTE) {
  ...
}

to be cleaner.

Even cooler would be without any branching, like,

ASSERT(ASCII == 0 && TWO_BYTE == 1);
ASSERT(state_ == ASCII || state_ == TWO_BYTE);
return buffer_.length() >> state_;

(Make sure the enum elements have values explicitly assigned.)

https://codereview.chromium.org/11299163/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11299163/diff/1/src/runtime.cc#newcode2860
src/runtime.cc:2860: zone);
Since we are doing the IsAscii() check (implied in Length()) in any case,
couldn't we leave it with

if (pattern_content.IsAscii()) {
  Vector<const char> pattern_vector = pattern_content.ToAsciiVector();
  if (pattern_vector.length() == 1) {
    ...
  } else {
    ...
  }
} else {
  Vector<const uc16> pattern_vector = pattern_content.ToUC16Vector();
  if (pattern_vector.length() == 1) {
    ...
  } else {
    ...
  }
}

I know this is a bit more verbose, but this way we don't have to introduce
Length() and also avoid the redundant ascii check.

[Yang, 2012-12-03 16:47:32]

On 2012/11/26 08:45:30, Yang wrote:
> Thanks for this patch! I have some drive-by comments.
> 
> https://codereview.chromium.org/11299163/diff/1/src/objects.h
> File src/objects.h (right):
> 
> https://codereview.chromium.org/11299163/diff/1/src/objects.h#newcode7195
> src/objects.h:7195: if (state_ == TWO_BYTE) return buffer_.length() / 2;
> you could do an
> if (state_ == ASCII) {
>   ...
> } else {
>   ASSERT(state == TWO_BYTE) {
>   ...
> }
> 
> to be cleaner.
> 
> Even cooler would be without any branching, like,
> 
> ASSERT(ASCII == 0 && TWO_BYTE == 1);
> ASSERT(state_ == ASCII || state_ == TWO_BYTE);
> return buffer_.length() >> state_;
> 
> (Make sure the enum elements have values explicitly assigned.)
> 
> https://codereview.chromium.org/11299163/diff/1/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/11299163/diff/1/src/runtime.cc#newcode2860
> src/runtime.cc:2860: zone);
> Since we are doing the IsAscii() check (implied in Length()) in any case,
> couldn't we leave it with
> 
> if (pattern_content.IsAscii()) {
>   Vector<const char> pattern_vector = pattern_content.ToAsciiVector();
>   if (pattern_vector.length() == 1) {
>     ...
>   } else {
>     ...
>   }
> } else {
>   Vector<const uc16> pattern_vector = pattern_content.ToUC16Vector();
>   if (pattern_vector.length() == 1) {
>     ...
>   } else {
>     ...
>   }
> }
> 
> I know this is a bit more verbose, but this way we don't have to introduce
> Length() and also avoid the redundant ascii check.

LGTM. I'll just land this CL with my suggested changes. :)