Flatten before String::WriteUtf8

Flatten before String::WriteUtf8

[ry, 2010-04-06 06:04:51]

Flattening the string before doing the WriteUtf8 speeds up some of my tests
significantly. (Granted, they use very large strings with made by many
concatenations.) I suspect there is a good reason why this isn't being done
currently, but I thought I'd submit this anyway.

[antonm, 2010-04-06 10:38:54]

I believe it was me who removed this flattening, see
http://code.google.com/p/v8/source/detail?r=2999

The idea was that flattening is worth if you write the same string several
times.  Otherwise, if you write it out only once, it's cheaper to bypass
flattening.

And overall, it seems right not to bundle two things---writing out and
flattening---into one function.

So could I suggest you to try one of the following approaches:

1) in your code, flat string before writing it out;
2) if for some reason that won't work, couldn't we add a flag if string should
be made flat (this flag should probably default to false).

What do you think?

[ry, 2010-04-06 16:59:02]

On 2010/04/06 10:38:54, antonm wrote:
> I believe it was me who removed this flattening, see
> http://code.google.com/p/v8/source/detail?r=2999
> 
> The idea was that flattening is worth if you write the same string several
> times.  Otherwise, if you write it out only once, it's cheaper to bypass
> flattening.
> 
> And overall, it seems right not to bundle two things---writing out and
> flattening---into one function.
> 
> So could I suggest you to try one of the following approaches:
> 
> 1) in your code, flat string before writing it out;
> 2) if for some reason that won't work, couldn't we add a flag if string should
> be made flat (this flag should probably default to false).
> 
> What do you think?

It would be nice to have V8 detect that a string is being written out multiple
times and flatten it automatically. I'm also fine with flattening the string
manually but there doesn't seem to be an API call for it?

Is there a reason why WriteAscii flattens?

[antonm, 2010-04-07 11:37:11]

http://codereview.chromium.org/1594017/diff/8001/9002
File src/api.cc (right):

http://codereview.chromium.org/1594017/diff/8001/9002#newcode2731
src/api.cc:2731: EnsureInitialized("v8::String::Flatten()");
I'd add ON_BAILOUT as well (or IsDeadCheck) as this operation has side-effects,
not a pure query.

http://codereview.chromium.org/1594017/diff/8001/9003
File test/cctest/test-strings.cc (right):

http://codereview.chromium.org/1594017/diff/8001/9003#newcode357
test/cctest/test-strings.cc:357: v8::Local<v8::String> cons =
v8::String::Concat(a,b);
I'd suspect that would create a flat string: v8::String::Concat delegates to
v8::internal::Factory::NewConsString which calls Heap::AllocateConsString.

This function
(https://cs.corp.google.com/p#chrome/trunk/src/v8/src/heap.cc&q=AllocateConsSt...)
would flatten result if its length is less than String::kMinNonFlatLength which
is currently 13 (see objects.h).

So you should probably pick longer strings and maybe assert that result string
is cons.

http://codereview.chromium.org/1594017/diff/8001/9003#newcode364
test/cctest/test-strings.cc:364: for (i = 0; i < 3; i++)
by no mean insisting, but as you don't reuse i after first for-loop, I'd rather
wrote it for (int i = ...).  Up to you.

http://codereview.chromium.org/1594017/diff/8001/9003#newcode365
test/cctest/test-strings.cc:365: CHECK_EQ(stringA[i], buffer[i]);
nit: current v8 styles is to either keep it in one line---for (...)
CHECK_EQ(...) or put into block:

for (...) {
  CHECK_EQ(...);
}

http://codereview.chromium.org/1594017/diff/8001/9003#newcode368
test/cctest/test-strings.cc:368: CHECK_EQ(stringB[i], buffer[i+3]);
ditto

[ry, 2010-04-07 18:58:44]

I addressed these issues.

[antonm, 2010-04-08 11:23:41]

Thanks a lot, Ryan.

LGTM and I going to land it, but there are some small amendments I could do on
landing.

http://codereview.chromium.org/1594017/diff/5002/14003
File test/cctest/test-strings.cc (right):

http://codereview.chromium.org/1594017/diff/5002/14003#newcode360
test/cctest/test-strings.cc:360: CHECK_EQ(false, str->IsFlat());
would you mind if I change that to CHECK(!str->IsFlat())?

http://codereview.chromium.org/1594017/diff/5002/14003#newcode364
test/cctest/test-strings.cc:364: CHECK_EQ(true, str->IsFlat());
would you mind if I change that to CHECK(str->IsFlat())?

http://codereview.chromium.org/1594017/diff/5002/14003#newcode369
test/cctest/test-strings.cc:369: for (int j = 0; j < 10; j++) {
why j, not i? :)  I could change to i when landing or could keep your j, up to
you.

[ry, 2010-04-08 14:11:02]

On 2010/04/08 11:23:41, antonm wrote:
> Thanks a lot, Ryan.
> 
> LGTM and I going to land it, but there are some small amendments I could do on
> landing.

Thanks!

> http://codereview.chromium.org/1594017/diff/5002/14003
> File test/cctest/test-strings.cc (right):
> 
> http://codereview.chromium.org/1594017/diff/5002/14003#newcode360
> test/cctest/test-strings.cc:360: CHECK_EQ(false, str->IsFlat());
> would you mind if I change that to CHECK(!str->IsFlat())?
>
> http://codereview.chromium.org/1594017/diff/5002/14003#newcode364
> test/cctest/test-strings.cc:364: CHECK_EQ(true, str->IsFlat());
> would you mind if I change that to CHECK(str->IsFlat())?

That's fine
 
> http://codereview.chromium.org/1594017/diff/5002/14003#newcode369
> test/cctest/test-strings.cc:369: for (int j = 0; j < 10; j++) {
> why j, not i? :)  I could change to i when landing or could keep your j, up to
> you.

Oh, I just changed that when I discovered the i:: internal namespace alias. It
can be changed back.