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 525dea2..08ee046 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 1e949a2..4206d29 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 d03f5f7..895e245 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 5e570f3..6340da5 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 {