Revert of Sampling heap profiler data structure changes (patchset #10...

Revert of Sampling heap profiler data structure changes (patchset #10 id:180001 of https://codereview.chromium.org/1697903002/ )

Reason for revert:
[Sheriff] Speculative revert for cpu profiler crashes on chromebooks:
https://build.chromium.org/p/client.v8/builders/V8%20Arm%20-%20debug/builds/549
https://build.chromium.org/p/client.v8/builders/V8%20Arm%20-%20debug/builds/550

Original issue's description:
> Sampling heap profiler data structure changes
>
> Previously, the sampling heap profiler stored a list of samples and then
> built a tree representation when the profile was queried by calling
> GetAllocationProfile. This change reduces duplication by removing stacks
> from all samples. Also, less information is stored in the tree
> maintained by the profiler and remaining information (script name, line
> no, etc) is resolved when a profile is requested.
>
> BUG=
>
> Committed: https://crrev.com/cdd55e2a3717723492d76f66810bf56b8de7f198
> Cr-Commit-Position: refs/heads/master@{#34119}

TBR=ofrobots@google.com,ulan@chromium.org,hpayer@chromium.org,mattloring@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review URL: https://codereview.chromium.org/1708363002

Cr-Commit-Position: refs/heads/master@{#34128}

src/profiler/sampling-heap-profiler.cc

@@ -44,7 +44,6 @@ SamplingHeapProfiler::SamplingHeapProfiler(Heap* heap, StringsStorage* names,
           heap_, static_cast<intptr_t>(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());
@@ -66,10 +65,12 @@ SamplingHeapProfiler::~SamplingHeapProfiler() {
     }
   }
 
-  for (auto sample : samples_) {
-    delete sample;
+  // 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;
   }
-  std::set<Sample*> empty;
+  std::set<SampledAllocation*> empty;
   samples_.swap(empty);
 }
 
@@ -87,48 +88,51 @@ void SamplingHeapProfiler::SampleObject(Address soon_object, size_t size) {
 
   Local<v8::Value> loc = v8::Utils::ToLocal(obj);
 
-  AllocationNode* node = AddStack();
-  node->allocations_[size]++;
-  Sample* sample = new Sample(size, node, loc, this);
+  SampledAllocation* sample =
+      new SampledAllocation(this, isolate_, loc, size, stack_depth_);
   samples_.insert(sample);
-  sample->global.SetWeak(sample, OnWeakCallback, WeakCallbackType::kParameter);
 }
 
-void SamplingHeapProfiler::OnWeakCallback(
-    const WeakCallbackInfo<Sample>& data) {
-  Sample* sample = data.GetParameter();
-  AllocationNode* node = sample->owner;
-  DCHECK(node->allocations_[sample->size] > 0);
-  node->allocations_[sample->size]--;
-  sample->profiler->samples_.erase(sample);
+
+void SamplingHeapProfiler::SampledAllocation::OnWeakCallback(
+    const WeakCallbackInfo<SampledAllocation>& data) {
+  SampledAllocation* sample = data.GetParameter();
+  sample->sampling_heap_profiler_->samples_.erase(sample);
   delete sample;
 }
 
-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) {
-      return child;
+
+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);
     }
   }
-  AllocationNode* child = new AllocationNode(name, script_id, start_position);
-  parent->children_.push_back(child);
-  return child;
 }
 
-SamplingHeapProfiler::AllocationNode* SamplingHeapProfiler::AddStack() {
-  AllocationNode* node = &profile_root_;
 
-  std::vector<SharedFunctionInfo*> stack;
-  StackTraceFrameIterator it(isolate_);
+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);
   int frames_captured = 0;
-  while (!it.done() && frames_captured < stack_depth_) {
+  while (!it.done() && frames_captured < max_frames) {
     JavaScriptFrame* frame = it.frame();
     SharedFunctionInfo* shared = frame->function()->shared();
-    stack.push_back(shared);
+    stack_.push_back(new FunctionInfo(shared, sampling_heap_profiler->names()));
 
     frames_captured++;
     it.Advance();
@@ -136,7 +140,7 @@ SamplingHeapProfiler::AllocationNode* SamplingHeapProfiler::AddStack() {
 
   if (frames_captured == 0) {
     const char* name = nullptr;
-    switch (isolate_->current_vm_state()) {
+    switch (isolate->current_vm_state()) {
       case GC:
         name = "(GC)";
         break;
@@ -156,63 +160,71 @@ SamplingHeapProfiler::AllocationNode* SamplingHeapProfiler::AddStack() {
         name = "(JS)";
         break;
     }
-    return FindOrAddChildNode(node, name, v8::UnboundScript::kNoScriptId, 0);
+    stack_.push_back(new FunctionInfo(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(""));
+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;
-  std::vector<v8::AllocationProfile::Allocation> allocations;
-  if (node->script_id_ != v8::UnboundScript::kNoScriptId) {
+
+  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);
-    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);
+    Handle<Script> script(non_const_scripts[function_info->get_script_id()]);
 
-    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});
-    }
+    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(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));
-  }
-  return current;
+      {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;
+  }
+  v8::AllocationProfile::Node* child =
+      AllocateNode(profile, scripts, function_info);
+  parent->children.push_back(child);
+  return child;
+}
+
+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.
@@ -227,7 +239,15 @@ v8::AllocationProfile* SamplingHeapProfiler::GetAllocationProfile() {
 
   auto profile = new v8::internal::AllocationProfile();
 
-  TranslateAllocationNode(profile, &profile_root_, scripts);
+  // 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});
+  }
 
   return profile;
 }

src/profiler/sampling-heap-profiler.h

@@ -48,50 +48,50 @@ class SamplingHeapProfiler {
 
   StringsStorage* names() const { return names_; }
 
-  class AllocationNode;
-
-  struct Sample {
+  class FunctionInfo {
    public:
-    Sample(size_t size_, AllocationNode* owner_, Local<Value> local_,
-           SamplingHeapProfiler* profiler_)
-        : size(size_),
-          owner(owner_),
-          global(Global<Value>(
-              reinterpret_cast<v8::Isolate*>(profiler_->isolate_), local_)),
-          profiler(profiler_) {}
-    ~Sample() { global.Reset(); }
-    const size_t size;
-    AllocationNode* const owner;
-    Global<Value> global;
-    SamplingHeapProfiler* const profiler;
+    FunctionInfo(SharedFunctionInfo* shared, StringsStorage* names);
+    explicit FunctionInfo(const char* name)
+        : name_(name),
+          script_name_(""),
+          script_id_(v8::UnboundScript::kNoScriptId),
+          start_position_(0) {}
+
+    const char* get_name() const { return name_; }
+    const char* get_script_name() const { return script_name_; }
+    int get_script_id() const { return script_id_; }
+    int get_start_position() const { return start_position_; }
 
    private:
-    DISALLOW_COPY_AND_ASSIGN(Sample);
+    const char* const name_;
+    const char* script_name_;
+    int script_id_;
+    const int start_position_;
   };
 
-  class AllocationNode {
+  class SampledAllocation {
    public:
-    AllocationNode(const char* const name, int script_id,
-                   const int start_position)
-        : script_id_(script_id),
-          script_position_(start_position),
-          name_(name) {}
-    ~AllocationNode() {
-      for (auto child : children_) {
-        delete child;
+    SampledAllocation(SamplingHeapProfiler* sampling_heap_profiler,
+                      Isolate* isolate, Local<Value> local, size_t size,
+                      int max_frames);
+    ~SampledAllocation() {
+      for (auto info : stack_) {
+        delete info;
       }
+      global_.Reset();  // drop the reference.
     }
+    size_t get_size() const { return size_; }
+    const std::vector<FunctionInfo*>& get_stack() const { return stack_; }
 
    private:
-    std::map<size_t, unsigned int> allocations_;
-    std::vector<AllocationNode*> children_;
-    const int script_id_;
-    const int script_position_;
-    const char* const name_;
+    static void OnWeakCallback(const WeakCallbackInfo<SampledAllocation>& data);
 
-    friend class SamplingHeapProfiler;
+    SamplingHeapProfiler* const sampling_heap_profiler_;
+    Global<Value> global_;
+    std::vector<FunctionInfo*> stack_;
+    const size_t size_;
 
-    DISALLOW_COPY_AND_ASSIGN(AllocationNode);
+    DISALLOW_COPY_AND_ASSIGN(SampledAllocation);
   };
 
  private:
@@ -99,29 +99,23 @@ class SamplingHeapProfiler {
 
   void SampleObject(Address soon_object, size_t size);
 
-  static void OnWeakCallback(const WeakCallbackInfo<Sample>& data);
-
   // Methods that construct v8::AllocationProfile.
-
-  // Translates the provided AllocationNode *node* returning an equivalent
-  // AllocationProfile::Node. The newly created AllocationProfile::Node is added
-  // to the provided AllocationProfile *profile*. Line numbers, column numbers,
-  // and script names are resolved using *scripts* which maps all currently
-  // loaded scripts keyed by their script id.
-  v8::AllocationProfile::Node* TranslateAllocationNode(
-      AllocationProfile* profile, SamplingHeapProfiler::AllocationNode* node,
-      const std::map<int, Script*>& scripts);
-  AllocationNode* AddStack();
-  AllocationNode* FindOrAddChildNode(AllocationNode* parent, const char* name,
-                                     int script_id, int start_position);
+  v8::AllocationProfile::Node* AddStack(
+      AllocationProfile* profile, const std::map<int, Script*>& scripts,
+      const std::vector<FunctionInfo*>& stack);
+  v8::AllocationProfile::Node* FindOrAddChildNode(
+      AllocationProfile* profile, const std::map<int, Script*>& scripts,
+      v8::AllocationProfile::Node* parent, FunctionInfo* function_info);
+  v8::AllocationProfile::Node* AllocateNode(
+      AllocationProfile* profile, const std::map<int, Script*>& scripts,
+      FunctionInfo* function_info);
 
   Isolate* const isolate_;
   Heap* const heap_;
   base::SmartPointer<SamplingAllocationObserver> new_space_observer_;
   base::SmartPointer<SamplingAllocationObserver> other_spaces_observer_;
   StringsStorage* const names_;
-  AllocationNode profile_root_;
-  std::set<Sample*> samples_;
+  std::set<SampledAllocation*> samples_;
   const int stack_depth_;
 
   friend class SamplingAllocationObserver;