Allow the morphing of strings to external strings to avoid having to...

Allow the morphing of strings to external strings to avoid having to
create copies in the embedding code (aka WebKit V8 bindings) on every
external use.


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

[iposva, 2009-02-06 01:42:10]

I would like to get feedback on this change before everybody disappears on
vacation please. There is a companion change coming with the associated V8
bindings change in WebKit.

Unfinished parts:
- Removing external symbols from the symbol_table when they get collected.
- Only two-byte external strings are currently handled.

Thanks,
-Ivan

[Erik Corry, 2009-02-06 08:04:23]

Looks good, the bits I understand.

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

http://codereview.chromium.org/21117/diff/1/3#newcode2442
Line 2442: void* parameter) {
I don't understand this to be honest.

http://codereview.chromium.org/21117/diff/1/3#newcode2536
Line 2536: // Make sure the GC calls the external destructor when no references
remain.
Do we do this even when 'result' is false?

http://codereview.chromium.org/21117/diff/1/5
File src/objects.cc (right):

http://codereview.chromium.org/21117/diff/1/5#newcode688
Line 688: ExternalTwoByteString* thsi = ExternalTwoByteString::cast(this);
Somewhat difficult to spot this variable name.  I usually prefer 'self', but
something more descriptive might be better.

http://codereview.chromium.org/21117/diff/1/5#newcode703
Line 703: HeapObject* filler = reinterpret_cast<HeapObject*>(filler_addr);
'this' is tagged, so filler_addr is also tagged?    I think HeapObjects have an
address method and there's a fromAddress method for going the other way.  That
seems less confusing to than having a variable named *address that is tagged.

http://codereview.chromium.org/21117/diff/1/5#newcode712
Line 712: WRITE_BARRIER(filler, HeapObject::kMapOffset);
You don't need this since the map is never in new space.

[iposva, 2009-02-06 08:18:40]

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

http://codereview.chromium.org/21117/diff/1/3#newcode2442
Line 2442: void* parameter) {
On 2009/02/06 08:04:24, Erik Corry wrote:
> I don't understand this to be honest.

Can you be more specific about what you don't understand? Then I can add the
appropriate comments.

http://codereview.chromium.org/21117/diff/1/3#newcode2536
Line 2536: // Make sure the GC calls the external destructor when no references
remain.
On 2009/02/06 08:04:24, Erik Corry wrote:
> Do we do this even when 'result' is false?

Excellent point. Will fix.

http://codereview.chromium.org/21117/diff/1/5
File src/objects.cc (right):

http://codereview.chromium.org/21117/diff/1/5#newcode688
Line 688: ExternalTwoByteString* thsi = ExternalTwoByteString::cast(this);
On 2009/02/06 08:04:24, Erik Corry wrote:
> Somewhat difficult to spot this variable name.  I usually prefer 'self', but
> something more descriptive might be better.

How about using "dies"? ;-)

http://codereview.chromium.org/21117/diff/1/5#newcode703
Line 703: HeapObject* filler = reinterpret_cast<HeapObject*>(filler_addr);
On 2009/02/06 08:04:24, Erik Corry wrote:
> 'this' is tagged, so filler_addr is also tagged?    I think HeapObjects have
an
> address method and there's a fromAddress method for going the other way.  That
> seems less confusing to than having a variable named *address that is tagged.
> 

There is a comment at the beginning of the function that states that all
addresses are tagged. In any case the off by one shouldn't matter because the
addresses are only used when calculating offsets and not to access memory. In
any case I notice now that this comment is far off the use of address
calculations. Overall, I was able to simplify the code a lot from its initial
version, so there is now only one address calculation. I will see if I can use
address and fromAddress.

http://codereview.chromium.org/21117/diff/1/5#newcode712
Line 712: WRITE_BARRIER(filler, HeapObject::kMapOffset);
On 2009/02/06 08:04:24, Erik Corry wrote:
> You don't need this since the map is never in new space.

Good point. I "stole" this code from somewhere else. I should probably factor
this functionality out into a Heap::CreateFillerAt(Address addr, int size), fix
the WRITE_BARRIER and then make use of it in the other code as well.

[Erik Corry, 2009-02-06 08:37:14]

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

http://codereview.chromium.org/21117/diff/1/3#newcode2442
Line 2442: void* parameter) {
On 2009/02/06 08:18:40, iposva wrote:
> On 2009/02/06 08:04:24, Erik Corry wrote:
> > I don't understand this to be honest.
> 
> Can you be more specific about what you don't understand? Then I can add the
> appropriate comments.

It's not your changes, it's the method as a whole.  This was just a sort of
warning not to regard this method as reviewed.  I'm sure someone who understands
what it did before will understand what it does now.

[Mads Ager (chromium), 2009-02-06 08:38:22]

Look good. I have two issues:

1. We probably need to stop using StringShapes when we make this change.

2. I'm not sure I understand the part about calling DisposeExternalString when
it is still being referenced by the symbol table.  This seems like something we
should disallow, but maybe there is a good reason for it?

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

http://codereview.chromium.org/21117/diff/1/3#newcode2441
Line 2441: static void DisposeExternalString(v8::Persistent<v8::Value> obj,
I don't understand the TODO here either.   It seems to me that
DisposeExternalString should only be called after the string has been removed
from the symbol table by the pruning that takes place in mark-compact.cc?  Is
there an issue with that code when there are weak global handles involved? Or is
there something else going on here?

http://codereview.chromium.org/21117/diff/1/4
File src/heap.cc (right):

http://codereview.chromium.org/21117/diff/1/4#newcode2059
Line 2059: return short_external_symbol_map();
Good catch!

http://codereview.chromium.org/21117/diff/1/5
File src/objects.cc (right):

http://codereview.chromium.org/21117/diff/1/5#newcode670
Line 670: bool String::MakeExternal(v8::String::ExternalStringResource*
resource) {
You might experience issues after making this change.  Mutating strings in-place
will effect other code because some code might be holding on to a StringShape to
avoid repeatedly figuring out which type of string is being worked on. 
Currently, the only thing that can make a string change shape is a GC, so it is
unsafe to hold on to a StringShape across a GC.  Now it will become unsafe to
hold on to a StringShape if you do anything that might cause a callback.  This
will be hard to track.

I think if we do this, we probably have to stop using StringShapes.  I don't
remember how much of a win StringShapes were, so we should measure it again.

http://codereview.chromium.org/21117/diff/1/5#newcode691
Line 691: // Additionally make the object into an external symbol if
Unfinished comment.

[Erik Corry, 2009-02-06 08:46:33]

http://codereview.chromium.org/21117/diff/1/5
File src/objects.cc (right):

http://codereview.chromium.org/21117/diff/1/5#newcode670
Line 670: bool String::MakeExternal(v8::String::ExternalStringResource*
resource) {
Any callback can cause a GC so no change there.  But we should probably remove
StringShape anyway.  I regard StringShape as an error-prone experiment that
didn't work out.  Despite encouraging results when a preliminary VM was placed
on the benchmark server the end result was practically zero.  The issue may be
subtle compiler dependencies and that the extra register pressure causes spills
that negate the benefit of not having to reload maps constantly.

[Mads Ager (chromium), 2009-02-06 08:48:31]

> Any callback can cause a GC so no change there. 

Of course.  Thanks Erik! :-)

[iposva, 2009-02-06 09:13:56]

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

http://codereview.chromium.org/21117/diff/1/3#newcode2441
Line 2441: static void DisposeExternalString(v8::Persistent<v8::Value> obj,
On 2009/02/06 08:38:22, Mads Ager wrote:
> I don't understand the TODO here either.   It seems to me that
> DisposeExternalString should only be called after the string has been removed
> from the symbol table by the pruning that takes place in mark-compact.cc?  Is
> there an issue with that code when there are weak global handles involved? Or
is
> there something else going on here?

The TODO is for me to remember to address this code before submitting it and to
communicate the intention of the code while we go through a code-prereview.

This function is a weak handle callback which gets called when the handle
transitions from WEAK to NEAR_DEATH. Since the object is passed as a parameter
to the callback, the object cannot be collected in this GC cycle, invoking the
callback just indicates that this object was not reachable by any strong
references. Potentially the callback could make the object strongly reachable
again.

In any case the symbol table pruning only happens when the object is collected
and the space is available for reuse. This leads to the fact that we can have
external symbols in the symbol table where the external resource has been
released.

An alternative solution to the one I sketched out here would be that external
symbols are not handled by this weak callback handler and instead the external
resource is deleted from within the symbol table pruning code.

http://codereview.chromium.org/21117/diff/1/5
File src/objects.cc (right):

http://codereview.chromium.org/21117/diff/1/5#newcode691
Line 691: // Additionally make the object into an external symbol if
On 2009/02/06 08:38:22, Mads Ager wrote:
> Unfinished comment.

Will fix.

[Feng Qian, 2009-02-11 19:14:58]

Overall looks good to me.

You probably want to get some stats about how many global handles created for
external strings in the symbol table.
If the number is large, dispose external resources when pruning the symbol table
might be more efficient.

I also see there are some lint errors.

http://codereview.chromium.org/21117/diff/30/1023
File include/v8.h (right):

http://codereview.chromium.org/21117/diff/30/1023#newcode844
Line 844: */
The description does not mention a subtlety in the implementation: if the
current string is an external string, it won't change the current string, and
the function returns true. Is it intended behavior?

Also, MakeExternal name sounds like a factory method, how about
'MorphToExternal'? 

After reading the implementation, now I understand that MakeExternal can only
morph the current string to an external string with the same characters. It
isn't clear from comments.

http://codereview.chromium.org/21117/diff/30/1024
File src/api.cc (right):

http://codereview.chromium.org/21117/diff/30/1024#newcode2514
Line 2514: if (this->IsExternal()) return true;  // Already an external string.
See my comments in v8.h

http://codereview.chromium.org/21117/diff/30/1025
File src/heap.cc (left):

http://codereview.chromium.org/21117/diff/30/1025#oldcode2072
Line 2072: if (map == short_external_string_map()) return
short_external_string_map();
Is this a bug in the old code? Why we never catch it? Can we have a test case
for it? (I don't mean in this CL, but a bug needs to be filed).

[iposva, 2009-02-11 22:42:40]

> You probably want to get some stats about how many global handles
> created for external strings in the symbol table.
> If the number is large, dispose external resources when pruning the
> symbol table might be more efficient.

I am not sure I understand your concern here.
External symbols can only be deleted when pruning the symbol table. Also for
strings that are already symbols we do not create a handle.

http://codereview.chromium.org/21117/diff/30/1023
File include/v8.h (right):

http://codereview.chromium.org/21117/diff/30/1023#newcode844
Line 844: */
On 2009/02/11 19:14:59, Feng Qian wrote:
> The description does not mention a subtlety in the implementation: if the
> current string is an external string, it won't change the current string, and
> the function returns true. Is it intended behavior?

The intended behaviour is to make this an external string. I changed the API
definition in such a way that the function returns true if the string was
successfully made external. That means if the conversion fails because either
the original string already was an external string or there was no room then the
result is false.

> Also, MakeExternal name sounds like a factory method, how about
> 'MorphToExternal'? 
> 
> After reading the implementation, now I understand that MakeExternal can only
> morph the current string to an external string with the same characters. It
> isn't clear from comments.

Done.

http://codereview.chromium.org/21117/diff/30/1025
File src/heap.cc (left):

http://codereview.chromium.org/21117/diff/30/1025#oldcode2072
Line 2072: if (map == short_external_string_map()) return
short_external_string_map();
On 2009/02/11 19:14:59, Feng Qian wrote:
> Is this a bug in the old code? Why we never catch it? Can we have a test case
> for it? (I don't mean in this CL, but a bug needs to be filed).

Added a test case. Yes this was a very long standing bug.