Don't externalize fresh strings.

Don't externalize fresh strings.

With the current API the embedder has to extrenalize a string each
time a string is encountered to avoid the cost of repeated character
copying/conversion. The issue here is that the externalization cost
itself is non-negligible (both in time and space) and should not be
paid for a rarely used string. This change is an attempt to predict a
string's usage frequency based on its freshness. A string is
considered fresh if it was recently allocated in the new space.

Committed: http://code.google.com/p/v8/source/detail?r=3878

[Mads Ager (chromium), 2010-02-15 13:31:26]

LGTM

I'm not sure about the limit though.  It will be random whether a new space
string is allocated before that limit or after it.  We already talked about this
offline, but wouldn't it be better to never externalize new space strings?

http://codereview.chromium.org/608006/diff/1/2
File src/api.cc (right):

http://codereview.chromium.org/608006/diff/1/2#newcode3135
src/api.cc:3135: // Determines whether a string is fresh, that is whether it's
Determines -> Estimates (based on how close it is to the new space top).

[Vitaly Repeshko, 2010-02-15 13:38:31]

On 2010/02/15 13:31:26, Mads Ager wrote:
> LGTM
> 
> I'm not sure about the limit though.  It will be random whether a new space
> string is allocated before that limit or after it.  We already talked about
this
> offline, but wouldn't it be better to never externalize new space strings?

Never externalizing new space strings would be a cleaner but more dangerous
approach. The problem with it is that the embedder can potentially convert a
large new space string many times, and so not externalizing it will make things
worse. The freshness limit is also a string length limit and will help to avoid
such problems.

> http://codereview.chromium.org/608006/diff/1/2
> File src/api.cc (right):
> 
> http://codereview.chromium.org/608006/diff/1/2#newcode3135
> src/api.cc:3135: // Determines whether a string is fresh, that is whether it's
> Determines -> Estimates (based on how close it is to the new space top).

Done.

[Vitaly Repeshko, 2010-02-15 22:29:07]

So to sum up our IM discussion, there are two issues here:
1. Large strings should be always externalized. This one was already addressed
in the initial patch. (>1K strings are unconditionally externalized.)
2. We should avoid randomly degrading performance when a frequently used string
stays close to the new space top because of the way GC was triggered. This one
is relatively unlikely, but can be bad. To guard from it I added a simple
String::Write invocation counter for fresh strings. (At first, I wanted a
separate counter for each of the strings, but then I realized that such
granularity is superfluous.) When this counter exceeds a certain value we will
satisfy next attempts to externalize.

Since the conditions to externalize became slightly more complex I added a
couple of tests.

Please take another look.


Thanks,
Vitaly

[Mads Ager (chromium), 2010-02-16 08:16:19]

LGTM

http://codereview.chromium.org/608006/diff/7001/7002
File src/api.cc (right):

http://codereview.chromium.org/608006/diff/7001/7002#newcode2467
src/api.cc:2467: // Records that the given string's characters were copied to
some
Could you update the comments in this class to make it clear that you do not
have a counter per string?

http://codereview.chromium.org/608006/diff/7001/7003
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/608006/diff/7001/7003#newcode474
test/cctest/test-api.cc:474: CHECK(small_string->CanMakeExternal());
small_ -> large_

http://codereview.chromium.org/608006/diff/7001/7003#newcode511
test/cctest/test-api.cc:511: CHECK(small_string->CanMakeExternal());
small_ -> large_

[Vitaly Repeshko, 2010-02-16 18:56:42]

Thanks! Submitted.


-- Vitaly

http://codereview.chromium.org/608006/diff/7001/7002
File src/api.cc (right):

http://codereview.chromium.org/608006/diff/7001/7002#newcode2467
src/api.cc:2467: // Records that the given string's characters were copied to
some
On 2010/02/16 08:16:19, Mads Ager wrote:
> Could you update the comments in this class to make it clear that you do not
> have a counter per string?

Done.

http://codereview.chromium.org/608006/diff/7001/7003
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/608006/diff/7001/7003#newcode474
test/cctest/test-api.cc:474: CHECK(small_string->CanMakeExternal());
On 2010/02/16 08:16:19, Mads Ager wrote:
> small_ -> large_

Oops. Done.

http://codereview.chromium.org/608006/diff/7001/7003#newcode511
test/cctest/test-api.cc:511: CHECK(small_string->CanMakeExternal());
On 2010/02/16 08:16:19, Mads Ager wrote:
> small_ -> large_

Ditto.