Sampling heap profiler data structure changes

src/profiler/sampling-heap-profiler.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/profiler/sampling-heap-profiler.h"
 
 #include <stdint.h>
 #include <memory>
 #include "src/api.h"
 #include "src/base/utils/random-number-generator.h"
@@ skipping 24 matching lines @@
 
 SamplingHeapProfiler::SamplingHeapProfiler(Heap* heap, StringsStorage* names,
                                            uint64_t rate, int stack_depth)
     : isolate_(heap->isolate()),
       heap_(heap),
       new_space_observer_(new SamplingAllocationObserver(
           heap_, rate, rate, this, heap->isolate()->random_number_generator())),
       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()) {
     if (space != heap->new_space()) {
       space->AddAllocationObserver(other_spaces_observer_.get());
     }
   }
 }
 
 
 SamplingHeapProfiler::~SamplingHeapProfiler() {
   heap_->new_space()->RemoveAllocationObserver(new_space_observer_.get());
   AllSpaces spaces(heap_);
   for (Space* space = spaces.next(); space != NULL; space = spaces.next()) {
     if (space != heap_->new_space()) {
       space->RemoveAllocationObserver(other_spaces_observer_.get());
     }
   }
 
-  // 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_;
 }
 
 
 void SamplingHeapProfiler::SampleObject(Address soon_object, size_t size) {
   DisallowHeapAllocation no_allocation;
 
   HandleScope scope(isolate_);
   HeapObject* heap_object = HeapObject::FromAddress(soon_object);
   Handle<Object> obj(heap_object, isolate_);
 
   // Mark the new block as FreeSpace to make sure the heap is iterable while we
   // are taking the sample.
   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.

+
+  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);
-  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::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();
   }
 
   if (frames_captured == 0) {
     const char* name = nullptr;
-    switch (isolate->current_vm_state()) {
+    switch (isolate_->current_vm_state()) {
       case GC:
         name = "(GC)";
         break;
       case COMPILER:
         name = "(COMPILER)";
         break;
       case OTHER:
         name = "(V8 API)";
         break;
       case EXTERNAL:
         name = "(EXTERNAL)";
         break;
       case IDLE:
         name = "(IDLE)";
         break;
       case JS:
         name = "(JS)";
         break;
     }
     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()]);
-
-    line =
-        1 + Script::GetLineNumber(script, function_info->get_start_position());
-    column = 1 + Script::GetColumnNumber(script,
-                                         function_info->get_start_position());
-  }
-
-  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();
+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;
 }
 
-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::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);
+    }
+  }
+}
+
+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
   // scripts. Build a map to allow fast mapping from script id to script.
   std::map<int, Script*> scripts;
   {
     Script::Iterator iterator(isolate_);
     Script* script;
     while ((script = iterator.Next())) {
       scripts[script->id()] = script;
     }
   }
 
   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;
 }
 
 
 }  // namespace internal
 }  // namespace v8