Sampling heap profiler data structure changes

src/profiler/sampling-heap-profiler.cc

Index: src/profiler/sampling-heap-profiler.cc
diff --git a/src/profiler/sampling-heap-profiler.cc b/src/profiler/sampling-heap-profiler.cc
index e65279ab8f9c1b2d85cbd5c633a4f0574038b21c..fee726dfb3aa19324c1f85e0cb831a8cfc3457bc 100644
--- a/src/profiler/sampling-heap-profiler.cc
+++ b/src/profiler/sampling-heap-profiler.cc
@@ -42,8 +42,9 @@ SamplingHeapProfiler::SamplingHeapProfiler(Heap* heap, StringsStorage* names,
       other_spaces_observer_(new SamplingAllocationObserver(
           heap_, rate, rate, this, heap->isolate()->random_number_generator())),
       names_(names),
-      samples_(),
       stack_depth_(stack_depth) {
+  FunctionInfo function_info("(root)");
+  profile_root_ = new SamplingHeapProfiler::AllocationNode(&function_info);

[ofrobots, 2016/02/14 23:57:42]

drop the SamplingHeapProfiler:: namespace prefix here. The compiler will resolve
this correctly in this context.

[mattloring, 2016/02/16 05:28:40]

Done.

   heap->new_space()->AddAllocationObserver(new_space_observer_.get());
   AllSpaces spaces(heap);
   for (Space* space = spaces.next(); space != NULL; space = spaces.next()) {
@@ -63,13 +64,7 @@ SamplingHeapProfiler::~SamplingHeapProfiler() {
     }
   }
 
-  // Clear samples and drop all the weak references we are keeping.
-  std::set<SampledAllocation*>::iterator it;
-  for (it = samples_.begin(); it != samples_.end(); ++it) {
-    delete *it;
-  }

[ofrobots, 2016/02/14 23:57:42]

Are we still clearing the weak references we have been holding when the
SamplingHeapProfiler is destructed? If not, this is going to leak memory in V8
because of the book-keeping needed by V8 to deal with weak handles. Also, weak
callbacks will get called later when you don't expect them (as the
SamplingHeapProfiler no longer exists) and may end up crashing.

[mattloring, 2016/02/16 05:28:40]

Globals should be cleaned up now.

-  std::set<SampledAllocation*> empty;
-  samples_.swap(empty);
+  delete profile_root_;
 }
 
 
@@ -85,52 +80,26 @@ void SamplingHeapProfiler::SampleObject(Address soon_object, size_t size) {
   heap()->CreateFillerObjectAt(soon_object, static_cast<int>(size));
 
   Local<v8::Value> loc = v8::Utils::ToLocal(obj);
+  Global<Value>* global =
+      new Global<Value>(reinterpret_cast<v8::Isolate*>(isolate_), loc);
 
-  SampledAllocation* sample =
-      new SampledAllocation(this, isolate_, loc, size, stack_depth_);
-  samples_.insert(sample);
-}
-
+  std::vector<FunctionInfo*> stack = CollectStack(stack_depth_);
+  SamplingHeapProfiler::AllocationNode* node = AddStack(stack);
+  node->allocations_[size]++;
+  Sample* sample = new Sample({size, node, global});

[ofrobots, 2016/02/14 23:57:42]

Prefereable to avoid list initialization. Prefer either a constructor, or
simply:
Sample* sample = {...};

[mattloring, 2016/02/16 05:28:40]

Doing it as {...} yields: error: excess elements in scalar initializer and
writing a constructor runs into problems with the lack of copy constructor in
Global. Is there a nice way to address either of these?

[ofrobots, 2016/02/17 17:48:21]

You will need to use a constructor that accepts the necessary arguments to build
a Global rather than passing a global. It is by design that Globals cannot be
copied or assigned.

[mattloring, 2016/02/17 17:59:47]

Done.

 
-void SamplingHeapProfiler::SampledAllocation::OnWeakCallback(
-    const WeakCallbackInfo<SampledAllocation>& data) {
-  SampledAllocation* sample = data.GetParameter();
-  sample->sampling_heap_profiler_->samples_.erase(sample);
-  delete sample;
+  global->SetWeak(sample, OnWeakCallback, WeakCallbackType::kParameter);
 }
 
-
-SamplingHeapProfiler::FunctionInfo::FunctionInfo(SharedFunctionInfo* shared,
-                                                 StringsStorage* names)
-    : name_(names->GetFunctionName(shared->DebugName())),
-      script_name_(""),
-      script_id_(v8::UnboundScript::kNoScriptId),
-      start_position_(shared->start_position()) {
-  if (shared->script()->IsScript()) {
-    Script* script = Script::cast(shared->script());
-    script_id_ = script->id();
-    if (script->name()->IsName()) {
-      Name* name = Name::cast(script->name());
-      script_name_ = names->GetName(name);
-    }
-  }
-}
-
-
-SamplingHeapProfiler::SampledAllocation::SampledAllocation(
-    SamplingHeapProfiler* sampling_heap_profiler, Isolate* isolate,
-    Local<Value> local, size_t size, int max_frames)
-    : sampling_heap_profiler_(sampling_heap_profiler),
-      global_(reinterpret_cast<v8::Isolate*>(isolate), local),
-      size_(size) {
-  global_.SetWeak(this, OnWeakCallback, WeakCallbackType::kParameter);
-
-  StackTraceFrameIterator it(isolate);
+std::vector<SamplingHeapProfiler::FunctionInfo*>
+SamplingHeapProfiler::CollectStack(int max_frames) {
+  std::vector<SamplingHeapProfiler::FunctionInfo*> stack_;
+  StackTraceFrameIterator it(isolate_);
   int frames_captured = 0;
   while (!it.done() && frames_captured < max_frames) {
     JavaScriptFrame* frame = it.frame();
     SharedFunctionInfo* shared = frame->function()->shared();
-    stack_.push_back(new FunctionInfo(shared, sampling_heap_profiler->names()));
+    stack_.push_back(new FunctionInfo(shared, this->names()));
 
     frames_captured++;
     it.Advance();
@@ -138,7 +107,7 @@ SamplingHeapProfiler::SampledAllocation::SampledAllocation(
 
   if (frames_captured == 0) {
     const char* name = nullptr;
-    switch (isolate->current_vm_state()) {
+    switch (isolate_->current_vm_state()) {
       case GC:
         name = "(GC)";
         break;
@@ -160,68 +129,96 @@ SamplingHeapProfiler::SampledAllocation::SampledAllocation(
     }
     stack_.push_back(new FunctionInfo(name));
   }
+  return stack_;
 }
 
-v8::AllocationProfile::Node* SamplingHeapProfiler::AllocateNode(
-    AllocationProfile* profile, const std::map<int, Script*>& scripts,
-    FunctionInfo* function_info) {
-  DCHECK(function_info->get_name());
-  DCHECK(function_info->get_script_name());
-
-  int line = v8::AllocationProfile::kNoLineNumberInfo;
-  int column = v8::AllocationProfile::kNoColumnNumberInfo;
-
-  if (function_info->get_script_id() != v8::UnboundScript::kNoScriptId) {
-    // Cannot use std::map<T>::at because it is not available on android.
-    auto non_const_scripts = const_cast<std::map<int, Script*>&>(scripts);
-    Handle<Script> script(non_const_scripts[function_info->get_script_id()]);
+void SamplingHeapProfiler::OnWeakCallback(
+    const WeakCallbackInfo<SamplingHeapProfiler::Sample>& data) {

[ofrobots, 2016/02/14 23:57:42]

You might be able to drop the SHP:: prefix in the argument list too.

[mattloring, 2016/02/16 05:28:39]

Done.

+  SamplingHeapProfiler::Sample* sample = data.GetParameter();
+  SamplingHeapProfiler::AllocationNode* node = sample->owner;

[ofrobots, 2016/02/14 23:57:42]

Drop the SHP:: prefix.

[mattloring, 2016/02/16 05:28:40]

Done.

+  node->allocations_[sample->size]--;
+  delete sample->global;
+  delete sample;
+}
 
-    line =
-        1 + Script::GetLineNumber(script, function_info->get_start_position());
-    column = 1 + Script::GetColumnNumber(script,
-                                         function_info->get_start_position());
+SamplingHeapProfiler::FunctionInfo::FunctionInfo(SharedFunctionInfo* shared,
+                                                 StringsStorage* names)
+    : name_(names->GetFunctionName(shared->DebugName())),
+      script_name_(""),

[ofrobots, 2016/02/14 23:57:42]

Do we ever use this field any more? Drop it if you don't need it.

[mattloring, 2016/02/16 15:10:54]

Done.

+      script_id_(v8::UnboundScript::kNoScriptId),
+      start_position_(shared->start_position()) {
+  if (shared->script()->IsScript()) {
+    Script* script = Script::cast(shared->script());
+    script_id_ = script->id();
+    if (script->name()->IsName()) {
+      Name* name = Name::cast(script->name());
+      script_name_ = names->GetName(name);
+    }
   }
-
-  profile->nodes().push_back(v8::AllocationProfile::Node(
-      {ToApiHandle<v8::String>(isolate_->factory()->InternalizeUtf8String(
-           function_info->get_name())),
-       ToApiHandle<v8::String>(isolate_->factory()->InternalizeUtf8String(
-           function_info->get_script_name())),
-       function_info->get_script_id(), function_info->get_start_position(),
-       line, column, std::vector<v8::AllocationProfile::Node*>(),
-       std::vector<v8::AllocationProfile::Allocation>()}));
-
-  return &profile->nodes().back();
 }
 
-v8::AllocationProfile::Node* SamplingHeapProfiler::FindOrAddChildNode(
-    AllocationProfile* profile, const std::map<int, Script*>& scripts,
-    v8::AllocationProfile::Node* parent, FunctionInfo* function_info) {
-  for (v8::AllocationProfile::Node* child : parent->children) {
-    if (child->script_id == function_info->get_script_id() &&
-        child->start_position == function_info->get_start_position())
+SamplingHeapProfiler::AllocationNode* SamplingHeapProfiler::FindOrAddChildNode(
+    SamplingHeapProfiler::AllocationNode* parent, FunctionInfo* function_info) {
+  for (SamplingHeapProfiler::AllocationNode* child : parent->children_) {

[ofrobots, 2016/02/14 23:57:42]

drop the prefix.

[mattloring, 2016/02/16 05:28:39]

Done.

+    if (child->script_id_ == function_info->get_script_id() &&
+        child->script_position_ == function_info->get_start_position())
       return child;
   }
-  v8::AllocationProfile::Node* child =
-      AllocateNode(profile, scripts, function_info);
-  parent->children.push_back(child);
+  SamplingHeapProfiler::AllocationNode* child =

[ofrobots, 2016/02/14 23:57:42]

Drop the SamplingHeapProfiler prefix.

[mattloring, 2016/02/16 05:28:40]

Done.

+      new AllocationNode(function_info);
+  parent->children_.push_back(child);
   return child;
 }
 
-v8::AllocationProfile::Node* SamplingHeapProfiler::AddStack(
-    AllocationProfile* profile, const std::map<int, Script*>& scripts,
+SamplingHeapProfiler::AllocationNode* SamplingHeapProfiler::AddStack(
     const std::vector<FunctionInfo*>& stack) {
-  v8::AllocationProfile::Node* node = profile->GetRootNode();
+  SamplingHeapProfiler::AllocationNode* node = profile_root_;

[ofrobots, 2016/02/14 23:57:42]

drop the prefix.

[mattloring, 2016/02/16 05:28:39]

Done.

 
   // We need to process the stack in reverse order as the top of the stack is
   // the first element in the list.
   for (auto it = stack.rbegin(); it != stack.rend(); ++it) {
     FunctionInfo* function_info = *it;
-    node = FindOrAddChildNode(profile, scripts, node, function_info);
+    node = FindOrAddChildNode(node, function_info);
   }
   return node;
 }
 
+v8::AllocationProfile::Node* SamplingHeapProfiler::GenerateProfile(
+    AllocationProfile* profile, SamplingHeapProfiler::AllocationNode* node,
+    const std::map<int, Script*>& scripts) {
+  Local<v8::String> script_name =
+      ToApiHandle<v8::String>(isolate_->factory()->InternalizeUtf8String(""));
+  int line = v8::AllocationProfile::kNoLineNumberInfo;
+  int column = v8::AllocationProfile::kNoColumnNumberInfo;
+  std::vector<v8::AllocationProfile::Allocation> allocations;

[ofrobots, 2016/02/14 23:57:42]

In a follow-on CL, it would be good to make the V8 API change to make a
AllocationProfile::Node hold a map from size to int instead of the vector of
allocations as you are doing internally.

[mattloring, 2016/02/16 05:28:40]

Yup, this change was in the original version of the CL but I think a follow-on
is more appropriate.

+  if (node->script_id_ != v8::UnboundScript::kNoScriptId) {
+    auto non_const_scripts = const_cast<std::map<int, Script*>&>(scripts);

[ofrobots, 2016/02/14 23:57:42]

You dropped the necessary comment explaining why these ugly const gymnastics are
necessary.

+    Script* script = non_const_scripts[node->script_id_];
+    if (script->name()->IsName()) {
+      Name* name = Name::cast(script->name());
+      script_name = ToApiHandle<v8::String>(
+          isolate_->factory()->InternalizeUtf8String(names_->GetName(name)));
+    }
+    Handle<Script> script_handle(script);
+
+    line = 1 + Script::GetLineNumber(script_handle, node->script_position_);
+    column = 1 + Script::GetColumnNumber(script_handle, node->script_position_);
+    for (auto alloc : node->allocations_) {
+      allocations.push_back({alloc.first, alloc.second});
+    }
+  }
+
+  profile->nodes().push_back(v8::AllocationProfile::Node(
+      {ToApiHandle<v8::String>(
+           isolate_->factory()->InternalizeUtf8String(node->name_)),
+       script_name, node->script_id_, node->script_position_, line, column,
+       std::vector<v8::AllocationProfile::Node*>(), allocations}));
+  v8::AllocationProfile::Node* current = &profile->nodes().back();
+  for (auto child : node->children_) {
+    current->children.push_back(GenerateProfile(profile, child, scripts));
+  }
+  return current;
+}
 
 v8::AllocationProfile* SamplingHeapProfiler::GetAllocationProfile() {
   // To resolve positions to line/column numbers, we will need to look up
@@ -237,15 +234,7 @@ v8::AllocationProfile* SamplingHeapProfiler::GetAllocationProfile() {
 
   auto profile = new v8::internal::AllocationProfile();
 
-  // Create the root node.
-  FunctionInfo function_info("(root)");
-  AllocateNode(profile, scripts, &function_info);
-
-  for (SampledAllocation* allocation : samples_) {
-    v8::AllocationProfile::Node* node =
-        AddStack(profile, scripts, allocation->get_stack());
-    node->allocations.push_back({allocation->get_size(), 1});
-  }
+  GenerateProfile(profile, profile_root_, scripts);
 
   return profile;
 }