Fix HeapSnapshotsDiff test, diff implementation, and a bug introduced

during snapshot size optimization.

Sorry, now I figured out that the diff implementation itself was also
incorrect.  Reachable nodes must be filtered from the beginning,
otherwise, an object that is already disconnected, but not discarded
yet, will not appear as a deleted (thankfully, this bug for some
reason had appeared on the x64 port.)

BUG=868
TEST=HeapSnapshotRootPreservedAfterSorting

Review URL: http://codereview.chromium.org/3531005

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@5570 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/profile-generator.cc b/src/profile-generator.cc
index 525dea2fbaa0cf71a010c316e6b58309250ad866..08ee046970fe657b8a3557ed34a956b25e8965ec 100644 (file)
--- a/src/profile-generator.cc
+++ b/src/profile-generator.cc
@@ -952,7 +952,7 @@ void HeapEntry::PaintAllReachable() {
 
 
 void HeapEntry::Print(int max_depth, int indent) {
-  OS::Print("%6d %6d %6d [%ld] ",
+  OS::Print("%6d %6d %6d [%llu] ",
             self_size(), ReachableSize(), RetainedSize(), id_);
   if (type() != kString) {
     OS::Print("%s %.40s\n", TypeAsString(), name_);
@@ -1237,7 +1237,7 @@ HeapSnapshot::HeapSnapshot(HeapSnapshotsCollection* collection,
       type_(type),
       title_(title),
       uid_(uid),
-      root_entry_index_(-1),
+      root_entry_(NULL),
       raw_entries_(NULL),
       entries_sorted_(false) {
   STATIC_ASSERT(
@@ -1276,11 +1276,11 @@ HeapEntry* HeapSnapshot::AddEntry(HeapObject* object,
                                   int children_count,
                                   int retainers_count) {
   if (object == kInternalRootObject) {
-    ASSERT(root_entry_index_ == -1);
-    root_entry_index_ = entries_.length();
+    ASSERT(root_entry_ == NULL);
     ASSERT(retainers_count == 0);
-    return AddEntry(
+    root_entry_ = AddEntry(
         HeapEntry::kInternal, "", 0, 0, children_count, retainers_count);
+    return root_entry_;
   } else if (object->IsJSFunction()) {
     JSFunction* func = JSFunction::cast(object);
     SharedFunctionInfo* shared = func->shared();
@@ -2095,6 +2095,11 @@ HeapSnapshotsComparator::~HeapSnapshotsComparator() {
 
 HeapSnapshotsDiff* HeapSnapshotsComparator::Compare(HeapSnapshot* snapshot1,
                                                     HeapSnapshot* snapshot2) {
+  snapshot1->ClearPaint();
+  snapshot1->root()->PaintAllReachable();
+  snapshot2->ClearPaint();
+  snapshot2->root()->PaintAllReachable();
+
   List<HeapEntry*>* entries1 = snapshot1->GetSortedEntriesList();
   List<HeapEntry*>* entries2 = snapshot2->GetSortedEntriesList();
   int i = 0, j = 0;
@@ -2103,8 +2108,14 @@ HeapSnapshotsDiff* HeapSnapshotsComparator::Compare(HeapSnapshot* snapshot1,
     uint64_t id1 = entries1->at(i)->id();
     uint64_t id2 = entries2->at(j)->id();
     if (id1 == id2) {
-      i++;
-      j++;
+      HeapEntry* entry1 = entries1->at(i++);
+      HeapEntry* entry2 = entries2->at(j++);
+      if (entry1->painted_reachable() != entry2->painted_reachable()) {
+        if (entry1->painted_reachable())
+          deleted_entries.Add(entry1);
+        else
+          added_entries.Add(entry2);
+      }
     } else if (id1 < id2) {
       HeapEntry* entry = entries1->at(i++);
       deleted_entries.Add(entry);
@@ -2122,35 +2133,17 @@ HeapSnapshotsDiff* HeapSnapshotsComparator::Compare(HeapSnapshot* snapshot1,
     added_entries.Add(entry);
   }
 
-  snapshot1->ClearPaint();
-  snapshot1->root()->PaintAllReachable();
-  snapshot2->ClearPaint();
-  snapshot2->root()->PaintAllReachable();
-  int reachable_deleted_entries = 0, reachable_added_entries = 0;
-  for (int i = 0; i < deleted_entries.length(); ++i) {
-    HeapEntry* entry = deleted_entries[i];
-    if (entry->painted_reachable()) ++reachable_deleted_entries;
-  }
-  for (int i = 0; i < added_entries.length(); ++i) {
-    HeapEntry* entry = added_entries[i];
-    if (entry->painted_reachable()) ++reachable_added_entries;
-  }
-
   HeapSnapshotsDiff* diff = new HeapSnapshotsDiff(snapshot1, snapshot2);
   diffs_.Add(diff);
-  diff->CreateRoots(reachable_added_entries, reachable_deleted_entries);
+  diff->CreateRoots(added_entries.length(), deleted_entries.length());
 
-  int del_child_index = 0, deleted_entry_index = 1;
   for (int i = 0; i < deleted_entries.length(); ++i) {
     HeapEntry* entry = deleted_entries[i];
-    if (entry->painted_reachable())
-      diff->AddDeletedEntry(del_child_index++, deleted_entry_index++, entry);
+    diff->AddDeletedEntry(i, i + 1, entry);
   }
-  int add_child_index = 0, added_entry_index = 1;
   for (int i = 0; i < added_entries.length(); ++i) {
     HeapEntry* entry = added_entries[i];
-    if (entry->painted_reachable())
-      diff->AddAddedEntry(add_child_index++, added_entry_index++, entry);
+    diff->AddAddedEntry(i, i + 1, entry);
   }
   return diff;
 }

diff --git a/src/profile-generator.h b/src/profile-generator.h
index 1e949a2cfb1106bff670df9a66331f47de390fe4..4206d29f0dd5d2676cd0197e7c6edbdffa9c815d 100644 (file)
--- a/src/profile-generator.h
+++ b/src/profile-generator.h
@@ -662,7 +662,7 @@ class HeapSnapshot {
   Type type() { return type_; }
   const char* title() { return title_; }
   unsigned uid() { return uid_; }
-  HeapEntry* root() { return entries_[root_entry_index_]; }
+  HeapEntry* root() { return root_entry_; }
 
   void AllocateEntries(
       int entries_count, int children_count, int retainers_count);
@@ -704,7 +704,7 @@ class HeapSnapshot {
   Type type_;
   const char* title_;
   unsigned uid_;
-  int root_entry_index_;
+  HeapEntry* root_entry_;
   char* raw_entries_;
   List<HeapEntry*> entries_;
   bool entries_sorted_;

diff --git a/test/cctest/cctest.status b/test/cctest/cctest.status
index d03f5f7a00c47f3536b5f1e1581168fc2147c52f..895e24539eec902ba51217ab5429e76ebc11d837 100644 (file)
--- a/test/cctest/cctest.status
+++ b/test/cctest/cctest.status
@@ -35,11 +35,6 @@ test-debug/DebuggerAgent: PASS, (PASS || FAIL) if $system == linux
 # BUG(382): Weird test. Can't guarantee that it never times out.
 test-api/ApplyInterruption: PASS || TIMEOUT
 
-# Bug (484): This test which we thought was originally corrected in r5236
-# is reappering. Disabled until bug in test is fixed. This only fails
-# when snapshot is on, so I am marking it PASS || FAIL
-test-heap-profiler/HeapSnapshotsDiff: PASS || FAIL
-
 # These tests always fail.  They are here to test test.py.  If
 # they don't fail then test.py has failed.
 test-serialize/TestThatAlwaysFails: FAIL

diff --git a/test/cctest/test-heap-profiler.cc b/test/cctest/test-heap-profiler.cc
index 5e570f34de3dc97a3eb8974ae24932de36e3fc44..6340da53d8ce2ac7c08d2465e9356175f20bdcf8 100644 (file)
--- a/test/cctest/test-heap-profiler.cc
+++ b/test/cctest/test-heap-profiler.cc
@@ -787,6 +787,7 @@ TEST(HeapSnapshotsDiff) {
   CompileAndRunScript(
       "function A() {}\n"
       "function B(x) { this.x = x; }\n"
+      "function A2(a) { for (var i = 0; i < a; ++i) this[i] = i; }\n"
       "var a = new A();\n"
       "var b = new B(a);");
   const v8::HeapSnapshot* snapshot1 =
@@ -795,7 +796,7 @@ TEST(HeapSnapshotsDiff) {
   CompileAndRunScript(
       "delete a;\n"
       "b.x = null;\n"
-      "var a = new A();\n"
+      "var a = new A2(20);\n"
       "var b2 = new B(a);");
   const v8::HeapSnapshot* snapshot2 =
       v8::HeapProfiler::TakeSnapshot(v8::String::New("s2"));
@@ -811,7 +812,7 @@ TEST(HeapSnapshotsDiff) {
     const v8::HeapGraphNode* node = prop->GetToNode();
     if (node->GetType() == v8::HeapGraphNode::kObject) {
       v8::String::AsciiValue node_name(node->GetName());
-      if (strcmp(*node_name, "A") == 0) {
+      if (strcmp(*node_name, "A2") == 0) {
         CHECK(IsNodeRetainedAs(node, v8::HeapGraphEdge::kProperty, "a"));
         CHECK(!found_A);
         found_A = true;
@@ -849,6 +850,19 @@ TEST(HeapSnapshotsDiff) {
 }
 
 
+TEST(HeapSnapshotRootPreservedAfterSorting) {
+  v8::HandleScope scope;
+  LocalContext env;
+  const v8::HeapSnapshot* snapshot =
+      v8::HeapProfiler::TakeSnapshot(v8::String::New("s"));
+  const v8::HeapGraphNode* root1 = snapshot->GetRoot();
+  const_cast<i::HeapSnapshot*>(reinterpret_cast<const i::HeapSnapshot*>(
+      snapshot))->GetSortedEntriesList();
+  const v8::HeapGraphNode* root2 = snapshot->GetRoot();
+  CHECK_EQ(root1, root2);
+}
+
+
 namespace v8 {
 namespace internal {