Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(933)

Issue 21117: Allow the morphing of strings to external strings to avoid having to... (Closed)

Created:
11 years, 10 months ago by iposva
Modified:
9 years, 3 months ago
CC:
v8-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 18

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 5

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+388 lines, -45 lines) Patch
M include/v8.h View 1 2 3 2 chunks +20 lines, -0 lines 0 comments Download
M src/api.cc View 1 2 3 4 3 chunks +83 lines, -10 lines 0 comments Download
M src/heap.h View 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M src/heap.cc View 1 2 3 4 chunks +26 lines, -22 lines 0 comments Download
M src/mark-compact.cc View 2 3 4 1 chunk +28 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 4 chunks +11 lines, -1 line 0 comments Download
M src/objects.cc View 1 2 3 4 2 chunks +90 lines, -12 lines 0 comments Download
M src/objects-inl.h View 1 2 3 2 chunks +56 lines, -0 lines 0 comments Download
M test/cctest/test-api.cc View 2 3 1 chunk +70 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
iposva
I would like to get feedback on this change before everybody disappears on vacation please. ...
11 years, 10 months ago (2009-02-06 01:42:10 UTC) #1
Erik Corry
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) ...
11 years, 10 months ago (2009-02-06 08:04:23 UTC) #2
iposva
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 ...
11 years, 10 months ago (2009-02-06 08:18:40 UTC) #3
Erik Corry
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: ...
11 years, 10 months ago (2009-02-06 08:37:14 UTC) #4
Mads Ager (chromium)
Look good. I have two issues: 1. We probably need to stop using StringShapes when ...
11 years, 10 months ago (2009-02-06 08:38:22 UTC) #5
Erik Corry
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 ...
11 years, 10 months ago (2009-02-06 08:46:33 UTC) #6
Mads Ager (chromium)
> Any callback can cause a GC so no change there. Of course. Thanks Erik! ...
11 years, 10 months ago (2009-02-06 08:48:31 UTC) #7
iposva
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 ...
11 years, 10 months ago (2009-02-06 09:13:56 UTC) #8
iposva
11 years, 10 months ago (2009-02-11 00:48:42 UTC) #9
Feng Qian
Overall looks good to me. You probably want to get some stats about how many ...
11 years, 10 months ago (2009-02-11 19:14:58 UTC) #10
iposva
11 years, 10 months ago (2009-02-11 22:42:40 UTC) #11
> 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.

Powered by Google App Engine
This is Rietveld 408576698