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

Issue 11315004: Count external string resources that were assigned to symbol strings. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by loislo
Modified:
1 year, 5 months ago
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

When we do native memory snapshot we counts only external string resources that have been added to heap->external_string_table_.
I found that not all the strings that have external resources were added to this table.
We do not add Symbols to the table probably because they have different lifetime.
But these symbols also could have external resources. As example it could happen when we assign innerHtml.
The simplest solution is to visit symbol_table too.

BUG=none
TEST=VisitExternalStrings
R=yurys

Patch Set 1 #

Patch Set 2 : external resource for a symbol will also be counted in external_string_table. #

Patch Set 3 : with minor test fix #

Patch Set 4 : with minor test fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -6 lines) Lint Patch
M src/api.cc View 1 2 chunks +2 lines, -2 lines 0 comments 0 errors Download
M test/cctest/test-api.cc View 1 2 3 chunks +18 lines, -4 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 9
loislo
1 year, 6 months ago #1
yurys
lgtm
1 year, 6 months ago #2
loislo
1 year, 6 months ago #3
Yang
On 2012/10/26 09:29:19, loislo wrote: It seems to me that none of the strings made ...
1 year, 6 months ago #4
loislo
new version was uploaded
1 year, 6 months ago #5
yurys
On 2012/10/26 17:42:55, loislo wrote: > new version was uploaded It would make sense to ...
1 year, 5 months ago #6
Yang
On 2012/10/29 06:24:57, Yury Semikhatsky wrote: > On 2012/10/26 17:42:55, loislo wrote: > > new ...
1 year, 5 months ago #7
Yang
On 2012/10/29 15:40:48, Yang wrote: > On 2012/10/29 06:24:57, Yury Semikhatsky wrote: > > On ...
1 year, 5 months ago #8
Yang
1 year, 5 months ago #9
On 2012/10/29 16:38:24, Yang wrote:
> On 2012/10/29 15:40:48, Yang wrote:
> > On 2012/10/29 06:24:57, Yury Semikhatsky wrote:
> > > On 2012/10/26 17:42:55, loislo wrote:
> > > > new version was uploaded
> > > 
> > > It would make sense to also add a test for the case that Yang mentioned
when
> > > existing external string is converted to a symbol.
> > 
> > I sensed some pitfalls in this issue, so I discussed it with Erik. Summary:
> > 
> > The reason we have an external string table (EST) is so that at a late phase
> (B)
> > of GC, we check which strings in the EST is alive, and call the dispose
> callback
> > on those that are not (and remove them from the EST).
> > 
> > In a prior phase (A) we also do that for strings in the symbol table: if we
> find
> > dead external strings in the symbol table, we call its dispose callback.
> > 
> > Consider an external string that has been turned into a symbol (case 1):
it's
> > both in the EST and in the symbol table. When it dies, both phases (A) and
(B)
> > stumble upon it and call its callback, but only A actually succeeds in
calling
> > the callback, since it's then set to NULL so that (B) ignores it.
> > 
> > A symbol that has been externalized (case 2) is only dealt with in (A),
since
> > it's not in the EST. The reason is that this way we can save space in the
EST.
> > 
> > Visiting both the symbol table and the EST potentionally overvisits strings
of
> > case 1.
> > 
> > The question is, is overvisiting a problem? If not, we can just do that. If
> yes,
> > we have some options:
> > 1) not deal with external strings in (A) and always add case 2 strings to
the
> > EST, which increases the required space in the EST.
> > 2) remove case 1 strings from the EST, which is kind of expensive
> > 3) visit strings in the EST first and ignore those that are symbols, since
> those
> > are in the symbol table, that we visit thereafter.
> > 
> > I strongly favor the third option. Sorry about the incorrect comment I gave
> > earlier!
> 
> I implemented option 3 in https://chromiumcodereview.appspot.com/11340010/

Closing this since the issue has been solved in r12841.
Sign in to reply to this message.

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