Chromium Code Reviews
Help | Chromium Project | Sign in
(875)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 2 months ago by iposva
Modified:
2 years, 8 months ago
CC:
v8-dev_googlegroups.com
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) Lint Patch
M include/v8.h View 1 2 3 2 chunks +20 lines, -0 lines 0 comments 2 errors Download
M src/api.cc View 1 2 3 4 3 chunks +83 lines, -10 lines 0 comments 0 errors Download
M src/heap.h View 2 3 1 chunk +4 lines, -0 lines 0 comments 0 errors Download
M src/heap.cc View 1 2 3 4 chunks +26 lines, -22 lines 0 comments 0 errors Download
M src/mark-compact.cc View 2 3 4 1 chunk +28 lines, -0 lines 0 comments 0 errors Download
M src/objects.h View 1 2 3 4 4 chunks +11 lines, -1 line 0 comments 0 errors Download
M src/objects.cc View 1 2 3 4 2 chunks +90 lines, -12 lines 0 comments 0 errors Download
M src/objects-inl.h View 1 2 3 2 chunks +56 lines, -0 lines 0 comments 0 errors Download
M test/cctest/test-api.cc View 2 3 1 chunk +70 lines, -0 lines 0 comments 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 11
iposva
I would like to get feedback on this change before everybody disappears on vacation please. ...
5 years, 2 months ago #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) ...
5 years, 2 months ago #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 ...
5 years, 2 months ago #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: ...
5 years, 2 months ago #4
Mads Ager (chromium)
Look good. I have two issues: 1. We probably need to stop using StringShapes when ...
5 years, 2 months ago #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 ...
5 years, 2 months ago #6
Mads Ager (chromium)
> Any callback can cause a GC so no change there. Of course. Thanks Erik! ...
5 years, 2 months ago #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 ...
5 years, 2 months ago #8
iposva
5 years, 2 months ago #9
Feng Qian
Overall looks good to me. You probably want to get some stats about how many ...
5 years, 2 months ago #10
iposva
5 years, 2 months ago #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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6