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..fafaa84dc8e91c265572dfa0ee24541c08d5b923 100644
--- a/src/profiler/sampling-heap-profiler.cc
+++ b/src/profiler/sampling-heap-profiler.cc
@@ -42,6 +42,7 @@ SamplingHeapProfiler::SamplingHeapProfiler(Heap* heap, StringsStorage* names,
       other_spaces_observer_(new SamplingAllocationObserver(
           heap_, rate, rate, this, heap->isolate()->random_number_generator())),
       names_(names),
+      profile_root_("(root)", v8::UnboundScript::kNoScriptId, 0),
       samples_(),
       stack_depth_(stack_depth) {
   heap->new_space()->AddAllocationObserver(new_space_observer_.get());
@@ -63,13 +64,10 @@ 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;
+  for (auto sample : samples_) {
+    delete sample;
   }
-  std::set<SampledAllocation*> empty;
-  samples_.swap(empty);
+  samples_.clear();

[ofrobots, 2016/02/18 01:02:24]

nit: It is more efficient to do a swap with an empty set. See the code on the
left.

[mattloring, 2016/02/18 01:30:04]

Done.

 }
 
 
@@ -86,51 +84,47 @@ void SamplingHeapProfiler::SampleObject(Address soon_object, size_t size) {
 
   Local<v8::Value> loc = v8::Utils::ToLocal(obj);
 
-  SampledAllocation* sample =
-      new SampledAllocation(this, isolate_, loc, size, stack_depth_);
+  AllocationNode* node = AddStack();
+  node->allocations_[size]++;
+  Sample* sample = new Sample(size, node, loc, this);
   samples_.insert(sample);
+  sample->global.SetWeak(sample, OnWeakCallback, WeakCallbackType::kParameter);
 }
 
-
-void SamplingHeapProfiler::SampledAllocation::OnWeakCallback(
-    const WeakCallbackInfo<SampledAllocation>& data) {
-  SampledAllocation* sample = data.GetParameter();
-  sample->sampling_heap_profiler_->samples_.erase(sample);
+void SamplingHeapProfiler::OnWeakCallback(
+    const WeakCallbackInfo<Sample>& data) {
+  Sample* sample = data.GetParameter();
+  AllocationNode* node = sample->owner;
+  node->allocations_[sample->size]--;

[ofrobots, 2016/02/18 01:02:24]

map::operator[] silently insert a new entry in the map if it doesn't already
exist. Add a DCHECK before this point to ensure node->allocations_[sample->size]
!= 0.

[mattloring, 2016/02/18 01:30:04]

Done.

+  sample->profiler->samples_.erase(sample);
   delete sample;
 }
 
-
-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::AllocationNode* SamplingHeapProfiler::FindOrAddChildNode(
+    AllocationNode* parent, const char* name, int script_id,
+    int start_position) {
+  for (AllocationNode* child : parent->children_) {
+    if (child->script_id_ == script_id &&
+        child->script_position_ == start_position &&
+        strcmp(child->name_, name) == 0) {

[ofrobots, 2016/02/18 01:02:24]

I think the earlier comment from Ulan was to DCHECK that the names match.

I don't think you should do strcmp here. This should indeed be a DCHECK. If you
ever run into a situation where the script id and start position are the same
but the name is different, there is something fundamentally wrong with our
understanding of the universe. The best action would be to abort. Hence a
DCHECK.

[mattloring, 2016/02/18 01:30:04]

Actually, it is possible for two scripts to have the same id but different
names. Right now our custom entries for (JS), (GC), ... all have a script_id of
kNoScriptId. The strcmp was added because we were incorrectly attributing ticks
to the wrong node.

+      return child;
     }
   }
+  AllocationNode* child = new AllocationNode(name, script_id, start_position);
+  parent->children_.push_back(child);
+  return child;
 }
 
+SamplingHeapProfiler::AllocationNode* SamplingHeapProfiler::AddStack() {
+  AllocationNode* node = &profile_root_;
 
-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<SharedFunctionInfo*> stack;
+  StackTraceFrameIterator it(isolate_);
   int frames_captured = 0;
-  while (!it.done() && frames_captured < max_frames) {
+  while (!it.done() && frames_captured < stack_depth_) {
     JavaScriptFrame* frame = it.frame();
     SharedFunctionInfo* shared = frame->function()->shared();
-    stack_.push_back(new FunctionInfo(shared, sampling_heap_profiler->names()));
+    stack.push_back(shared);
 
     frames_captured++;
     it.Advance();
@@ -138,7 +132,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;
@@ -158,71 +152,63 @@ SamplingHeapProfiler::SampledAllocation::SampledAllocation(
         name = "(JS)";
         break;
     }
-    stack_.push_back(new FunctionInfo(name));
+    return FindOrAddChildNode(node, name, v8::UnboundScript::kNoScriptId, 0);
   }
-}
 
-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());
+  // 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) {
+    SharedFunctionInfo* shared = *it;
+    const char* name = this->names()->GetFunctionName(shared->DebugName());
+    int script_id = v8::UnboundScript::kNoScriptId;
+    if (shared->script()->IsScript()) {
+      Script* script = Script::cast(shared->script());
+      script_id = script->id();
+    }
+    node = FindOrAddChildNode(node, name, script_id, shared->start_position());
+  }
+  return node;
+}
 
+v8::AllocationProfile::Node* SamplingHeapProfiler::TranslateAllocationNode(
+    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;
-
-  if (function_info->get_script_id() != v8::UnboundScript::kNoScriptId) {
+  std::vector<v8::AllocationProfile::Allocation> allocations;
+  if (node->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()]);
+    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, function_info->get_start_position());
-    column = 1 + Script::GetColumnNumber(script,
-                                         function_info->get_start_position());
+    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(
-           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())
-      return child;
+      {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(
+        TranslateAllocationNode(profile, child, scripts));
   }
-  v8::AllocationProfile::Node* child =
-      AllocateNode(profile, scripts, function_info);
-  parent->children.push_back(child);
-  return child;
+  return current;
 }
 
-v8::AllocationProfile::Node* SamplingHeapProfiler::AddStack(
-    AllocationProfile* profile, const std::map<int, Script*>& scripts,
-    const std::vector<FunctionInfo*>& stack) {
-  v8::AllocationProfile::Node* node = profile->GetRootNode();
-
-  // 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);
-  }
-  return node;
-}
-
-
 v8::AllocationProfile* SamplingHeapProfiler::GetAllocationProfile() {
   // To resolve positions to line/column numbers, we will need to look up
   // scripts. Build a map to allow fast mapping from script id to script.
@@ -237,15 +223,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});
-  }
+  TranslateAllocationNode(profile, &profile_root_, scripts);
 
   return profile;
 }