Count external string resources that were assigned to symbol strings. (issue 11315004)

[loi...@chromium.org]

Reviewers: yurys,

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


Please review this at https://chromiumcodereview.appspot.com/11315004/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/heap.cc
M test/cctest/test-api.cc


Index: src/heap.cc
diff --git a/src/heap.cc b/src/heap.cc
index
909e7605b0f3094bec5a69472e8a38ed7eed4f5b..a51c790f636c084bd121dbb79005375b11022f0a
100644
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -1586,6 +1586,7 @@ void
Heap::VisitExternalResources(v8::ExternalResourceVisitor* visitor) {
v8::ExternalResourceVisitor* visitor_;
} visitor_adapter(visitor);
external_string_table_.Iterate(&visitor_adapter);
+ symbol_table()->IterateElements(&visitor_adapter);
}


Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index
3be068009e275e53bb60903d812af6f841ab6069..fad9b7a2d3ccad4d60f46f18da7f96cd04cefc78
100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -14816,11 +14816,13 @@ THREADED_TEST(GetHeapStatistics) {

class VisitorImpl : public v8::ExternalResourceVisitor {
public:
- VisitorImpl(TestResource* r1, TestResource* r2)
+ VisitorImpl(TestResource* r1, TestResource* r2, TestResource* r3)
: resource1_(r1),
resource2_(r2),
+ resource3_(r3),
found_resource1_(false),
- found_resource2_(false) {}
+ found_resource2_(false),
+ found_resource3_(false) {}
virtual ~VisitorImpl() {}
virtual void VisitExternalString(v8::Handle<v8::String> string) {
if (!string->IsExternal()) {
@@ -14838,17 +14840,24 @@ class VisitorImpl : public
v8::ExternalResourceVisitor {
CHECK(!found_resource2_);
found_resource2_ = true;
}
+ if (resource3_ == resource) {
+ CHECK(!found_resource3_);
+ found_resource3_ = true;
+ }
}
void CheckVisitedResources() {
CHECK(found_resource1_);
CHECK(found_resource2_);
+ CHECK(found_resource3_);
}

private:
v8::String::ExternalStringResource* resource1_;
v8::String::ExternalStringResource* resource2_;
+ v8::String::ExternalStringResource* resource3_;
bool found_resource1_;
bool found_resource2_;
+ bool found_resource3_;
};

TEST(VisitExternalStrings) {
@@ -14860,12 +14869,17 @@ TEST(VisitExternalStrings) {
v8::Local<v8::String> string1 = v8::String::NewExternal(resource1);
TestResource* resource2 = new TestResource(two_byte_string);
v8::Local<v8::String> string2 = v8::String::NewExternal(resource2);
+ TestResource* resource3 = new TestResource(two_byte_string);
+ v8::Local<v8::String> symbol = v8::String::NewSymbol("a symbol string");
+ symbol->MakeExternal(resource3);

- // We need to add usages for string1 and string2 to avoid warnings in
GCC 4.7
+ // We need to add usages for string1, string2 and symbol string
+ // to avoid warnings in GCC 4.7
CHECK(string1->IsExternal());
CHECK(string2->IsExternal());
+ CHECK(symbol->IsExternal());

- VisitorImpl visitor(resource1, resource2);
+ VisitorImpl visitor(resource1, resource2, resource3);
v8::V8::VisitExternalResources(&visitor);
visitor.CheckVisitedResources();
}

[yu...@chromium.org]

lgtm



https://chromiumcodereview.appspot.com/11315004/

[loi...@chromium.org]

https://chromiumcodereview.appspot.com/11315004/

[yan...@chromium.org]

On 2012/10/26 09:29:19, loislo wrote:

It seems to me that none of the strings made external using
V8::MakeExternal is
added to the external string table, not only symbols. On the other hand, if
an
existing external string is converted to a symbol, it would be counted twice
since it's both in the external string table and in the symbol table.
I think the fix should be adding external strings produced by
V8::MakeExternal
to the external string table.

https://chromiumcodereview.appspot.com/11315004/

[loi...@chromium.org]

new version was uploaded


https://chromiumcodereview.appspot.com/11315004/