Revert of CPUProfiler: Push deopt reason further to ProfileNode. (pat…

…chset #1 id:1 of https://codereview.chromium.org/919953002/) Reason for revert: static initializers broke the build Original issue's description: > CPUProfiler: Push deopt reason further to ProfileNode. > > 1) create beefy RelocInfo table when cpu profiler is active, so if a function > was optimized when profiler was active RelocInfo would get separate DeoptInfo > for the each deopt case. > > 2) push DeoptInfo from CodeEntry to ProfileNode. > When deopt happens we put the info collected on #1 into CodeEntry and record stack sample. > On the sampling thread we grab the deopt data and append it to the corresponding ProfileNode deopts list. > > Sample profile dump. > [Top down]: > 0 (root) 0 #1 > 1 29 v8#2 > 5 test 29 v8#3 > 3 opt_function 29 v8#4 > deopted at 52 with reason 'not a heap number' > deopted at 71 with reason 'division by zero' > > BUG=452067 > LOG=n > > Committed: https://crrev.com/ce8701b247d3c6604f24f17a90c02d17b4417f54 > Cr-Commit-Position: refs/heads/master@{#26615} TBR=jarin@chromium.org,svenpanne@chromium.org,yurys@chromium.org,alph@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=452067 Review URL: https://codereview.chromium.org/915173005 Cr-Commit-Position: refs/heads/master@{#26616}
nubjs · Feb 12, 2015 · cb6ea14 · cb6ea14
1 parent ce8701b
commit cb6ea14
Show file tree

Hide file tree

Showing 22 changed files with 49 additions and 101 deletions.
diff --git a/src/arm/assembler-arm.cc b/src/arm/assembler-arm.cc
@@ -3392,7 +3392,7 @@ void Assembler::RecordComment(const char* msg) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
@@ -907,8 +907,7 @@ void LCodeGen::DeoptimizeIf(Condition condition, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/arm64/assembler-arm64.cc b/src/arm64/assembler-arm64.cc
@@ -3081,7 +3081,7 @@ void Assembler::RecordComment(const char* msg) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/arm64/lithium-codegen-arm64.cc b/src/arm64/lithium-codegen-arm64.cc
@@ -1071,8 +1071,7 @@ void LCodeGen::DeoptimizeBranch(
             entry, deopt_info, bailout_type, !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry->IsEquivalentTo(*jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/cpu-profiler-inl.h b/src/cpu-profiler-inl.h
@@ -38,7 +38,10 @@ void CodeDisableOptEventRecord::UpdateCodeMap(CodeMap* code_map) {
 
 void CodeDeoptEventRecord::UpdateCodeMap(CodeMap* code_map) {
   CodeEntry* entry = code_map->FindEntry(start);
-  if (entry != NULL) entry->set_deopt_info(deopt_reason, raw_position);
+  if (entry != NULL) {
+    entry->set_deopt_reason(deopt_reason);
+    entry->set_deopt_location(raw_position);
+  }
 }
 
 

diff --git a/src/deoptimizer.h b/src/deoptimizer.h
@@ -188,6 +188,14 @@ class Deoptimizer : public Malloced {
     DeoptInfo(int r, const char* m, DeoptReason d)
         : raw_position(r), mnemonic(m), deopt_reason(d) {}
 
+    bool operator==(const DeoptInfo& other) const {
+      return raw_position == other.raw_position &&
+             CStringEquals(mnemonic, other.mnemonic) &&
+             deopt_reason == other.deopt_reason;
+    }
+
+    bool operator!=(const DeoptInfo& other) const { return !(*this == other); }
+
     int raw_position;
     const char* mnemonic;
     DeoptReason deopt_reason;
@@ -206,7 +214,8 @@ class Deoptimizer : public Malloced {
 
     bool IsEquivalentTo(const JumpTableEntry& other) const {
       return address == other.address && bailout_type == other.bailout_type &&
-             needs_frame == other.needs_frame;
+             needs_frame == other.needs_frame &&
+             (!FLAG_trace_deopt || deopt_info == other.deopt_info);
     }
 
     Label label;

diff --git a/src/ia32/assembler-ia32.cc b/src/ia32/assembler-ia32.cc
@@ -2652,7 +2652,7 @@ void Assembler::RecordComment(const char* msg, bool force) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc
@@ -872,8 +872,7 @@ void LCodeGen::DeoptimizeIf(Condition cc, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/mips/assembler-mips.cc b/src/mips/assembler-mips.cc
@@ -2355,7 +2355,7 @@ void Assembler::RecordComment(const char* msg) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/mips/lithium-codegen-mips.cc b/src/mips/lithium-codegen-mips.cc
@@ -870,8 +870,7 @@ void LCodeGen::DeoptimizeIf(Condition condition, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/mips64/assembler-mips64.cc b/src/mips64/assembler-mips64.cc
@@ -2582,7 +2582,7 @@ void Assembler::RecordComment(const char* msg) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/mips64/lithium-codegen-mips64.cc b/src/mips64/lithium-codegen-mips64.cc
@@ -820,8 +820,7 @@ void LCodeGen::DeoptimizeIf(Condition condition, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/ppc/assembler-ppc.cc b/src/ppc/assembler-ppc.cc
@@ -2196,7 +2196,7 @@ void Assembler::RecordComment(const char* msg) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/ppc/lithium-codegen-ppc.cc b/src/ppc/lithium-codegen-ppc.cc
@@ -825,8 +825,7 @@ void LCodeGen::DeoptimizeIf(Condition cond, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/profile-generator-inl.h b/src/profile-generator-inl.h
@@ -25,7 +25,7 @@ CodeEntry::CodeEntry(Logger::LogEventsAndTags tag, const char* name,
       script_id_(v8::UnboundScript::kNoScriptId),
       no_frame_ranges_(NULL),
       bailout_reason_(kEmptyBailoutReason),
-      deopt_reason_(kNoDeoptReason),
+      deopt_reason_(kEmptyBailoutReason),
       deopt_location_(0),
       line_info_(line_info),
       instruction_start_(instruction_start) {}

diff --git a/src/profile-generator.cc b/src/profile-generator.cc
@@ -8,7 +8,6 @@
 
 #include "src/compiler.h"
 #include "src/debug.h"
-#include "src/deoptimizer.h"
 #include "src/global-handles.h"
 #include "src/sampler.h"
 #include "src/scopeinfo.h"
@@ -159,9 +158,7 @@ int JITLineInfoTable::GetSourceLineNumber(int pc_offset) const {
 
 const char* const CodeEntry::kEmptyNamePrefix = "";
 const char* const CodeEntry::kEmptyResourceName = "";
-const char* const CodeEntry::kEmptyBailoutReason =
-    GetBailoutReason(BailoutReason::kNoReason);
-const char* const CodeEntry::kNoDeoptReason = "";
+const char* const CodeEntry::kEmptyBailoutReason = "";
 
 
 CodeEntry::~CodeEntry() {
@@ -215,12 +212,6 @@ int CodeEntry::GetSourceLine(int pc_offset) const {
 }
 
 
-void ProfileNode::CollectDeoptInfo(CodeEntry* entry) {
-  deopt_infos_.Add(DeoptInfo(entry->deopt_reason(), entry->deopt_location()));
-  entry->clear_deopt_info();
-}
-
-
 ProfileNode* ProfileNode::FindChild(CodeEntry* entry) {
   HashMap::Entry* map_entry =
       children_.Lookup(entry, CodeEntryHash(entry), false);
@@ -232,15 +223,13 @@ ProfileNode* ProfileNode::FindChild(CodeEntry* entry) {
 ProfileNode* ProfileNode::FindOrAddChild(CodeEntry* entry) {
   HashMap::Entry* map_entry =
       children_.Lookup(entry, CodeEntryHash(entry), true);
-  ProfileNode* node = reinterpret_cast<ProfileNode*>(map_entry->value);
-  if (node == NULL) {
+  if (map_entry->value == NULL) {
     // New node added.
-    node = new ProfileNode(tree_, entry);
-    map_entry->value = node;
-    children_list_.Add(node);
+    ProfileNode* new_node = new ProfileNode(tree_, entry);
+    map_entry->value = new_node;
+    children_list_.Add(new_node);
   }
-  if (entry->has_deopt_info()) node->CollectDeoptInfo(entry);
-  return node;
+  return reinterpret_cast<ProfileNode*>(map_entry->value);
 }
 
 
@@ -279,21 +268,12 @@ bool ProfileNode::GetLineTicks(v8::CpuProfileNode::LineTick* entries,
 
 
 void ProfileNode::Print(int indent) {
-  base::OS::Print("%5u %*s %s%s %d #%d", self_ticks_, indent, "",
+  base::OS::Print("%5u %*s %s%s %d #%d %s", self_ticks_, indent, "",
                   entry_->name_prefix(), entry_->name(), entry_->script_id(),
-                  id());
+                  id(), entry_->bailout_reason());
   if (entry_->resource_name()[0] != '\0')
     base::OS::Print(" %s:%d", entry_->resource_name(), entry_->line_number());
   base::OS::Print("\n");
-  for (auto info : deopt_infos_) {
-    base::OS::Print("%*s deopted at %d with reason '%s'\n", indent + 10, "",
-                    info.deopt_location, info.deopt_reason);
-  }
-  const char* bailout_reason = entry_->bailout_reason();
-  if (bailout_reason != GetBailoutReason(BailoutReason::kNoReason)) {
-    base::OS::Print("%*s bailed out due to '%s'\n", indent + 10, "",
-                    bailout_reason);
-  }
   for (HashMap::Entry* p = children_.Start();
        p != NULL;
        p = children_.Next(p)) {

diff --git a/src/profile-generator.h b/src/profile-generator.h
@@ -90,21 +90,11 @@ class CodeEntry {
   void set_bailout_reason(const char* bailout_reason) {
     bailout_reason_ = bailout_reason;
   }
-  const char* bailout_reason() const { return bailout_reason_; }
-
-  void set_deopt_info(const char* deopt_reason, int location) {
-    DCHECK(deopt_reason_ == kNoDeoptReason);
-    DCHECK(!deopt_location_);
+  void set_deopt_reason(const char* deopt_reason) {
     deopt_reason_ = deopt_reason;
-    deopt_location_ = location;
-  }
-  const char* deopt_reason() const { return deopt_reason_; }
-  int deopt_location() const { return deopt_location_; }
-  bool has_deopt_info() const { return deopt_reason_ != kNoDeoptReason; }
-  void clear_deopt_info() {
-    deopt_reason_ = kNoDeoptReason;
-    deopt_location_ = 0;
   }
+  void set_deopt_location(int location) { deopt_location_ = location; }
+  const char* bailout_reason() const { return bailout_reason_; }
 
   static inline bool is_js_function_tag(Logger::LogEventsAndTags tag);
 
@@ -128,7 +118,6 @@ class CodeEntry {
   static const char* const kEmptyNamePrefix;
   static const char* const kEmptyResourceName;
   static const char* const kEmptyBailoutReason;
-  static const char* const kNoDeoptReason;
 
  private:
   class TagField : public BitField<Logger::LogEventsAndTags, 0, 8> {};
@@ -157,17 +146,6 @@ class CodeEntry {
 class ProfileTree;
 
 class ProfileNode {
- private:
-  struct DeoptInfo {
-    DeoptInfo(const char* deopt_reason, int deopt_location)
-        : deopt_reason(deopt_reason), deopt_location(deopt_location) {}
-    DeoptInfo(const DeoptInfo& info)
-        : deopt_reason(info.deopt_reason),
-          deopt_location(info.deopt_location) {}
-    const char* deopt_reason;
-    int deopt_location;
-  };
-
  public:
   inline ProfileNode(ProfileTree* tree, CodeEntry* entry);
 
@@ -184,8 +162,6 @@ class ProfileNode {
   unsigned int GetHitLineCount() const { return line_ticks_.occupancy(); }
   bool GetLineTicks(v8::CpuProfileNode::LineTick* entries,
                     unsigned int length) const;
-  void CollectDeoptInfo(CodeEntry* entry);
-  const List<DeoptInfo>& deopt_infos() const { return deopt_infos_; }
 
   void Print(int indent);
 
@@ -210,7 +186,6 @@ class ProfileNode {
   unsigned id_;
   HashMap line_ticks_;
 
-  List<DeoptInfo> deopt_infos_;
   DISALLOW_COPY_AND_ASSIGN(ProfileNode);
 };
 

diff --git a/src/x64/assembler-x64.cc b/src/x64/assembler-x64.cc
@@ -3451,7 +3451,7 @@ void Assembler::RecordComment(const char* msg, bool force) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc
@@ -782,8 +782,7 @@ void LCodeGen::DeoptimizeIf(Condition cc, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/src/x87/assembler-x87.cc b/src/x87/assembler-x87.cc
@@ -1921,7 +1921,7 @@ void Assembler::RecordComment(const char* msg, bool force) {
 
 
 void Assembler::RecordDeoptReason(const int reason, const int raw_position) {
-  if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling()) {
+  if (FLAG_trace_deopt) {
     EnsureSpace ensure_space(this);
     RecordRelocInfo(RelocInfo::POSITION, raw_position);
     RecordRelocInfo(RelocInfo::DEOPT_REASON, reason);

diff --git a/src/x87/lithium-codegen-x87.cc b/src/x87/lithium-codegen-x87.cc
@@ -1154,8 +1154,7 @@ void LCodeGen::DeoptimizeIf(Condition cc, LInstruction* instr,
                                             !frame_is_built_);
     // We often have several deopts to the same entry, reuse the last
     // jump entry if this is the case.
-    if (FLAG_trace_deopt || isolate()->cpu_profiler()->is_profiling() ||
-        jump_table_.is_empty() ||
+    if (jump_table_.is_empty() ||
         !table_entry.IsEquivalentTo(jump_table_.last())) {
       jump_table_.Add(table_entry, zone());
     }

diff --git a/test/cctest/test-cpu-profiler.cc b/test/cctest/test-cpu-profiler.cc
@@ -1720,31 +1720,23 @@ TEST(DontStopOnFinishedProfileDelete) {
 
 
 static const char* collect_deopt_events_test_source =
-    "function opt_function(left, right) {\n"
-    "  var k = left / 10;\n"
-    "  var r = 10 / right;\n"
-    "  return k + r;"
+    "function opt_function(value) {\n"
+    "  return value / 10;\n"
     "}\n"
     "\n"
-    "function test(left, right) {\n"
-    "  return opt_function(left, right);\n"
+    "function test(value) {\n"
+    "  return opt_function(value);\n"
     "}\n"
     "\n"
     "startProfiling();\n"
     "\n"
-    "test(10, 10);\n"
+    "for (var i = 0; i < 10; ++i) test(10);\n"
     "\n"
     "%OptimizeFunctionOnNextCall(opt_function)\n"
     "\n"
-    "test(10, 10);\n"
+    "for (var i = 0; i < 10; ++i) test(10);\n"
     "\n"
-    "test(undefined, 10);\n"
-    "\n"
-    "%OptimizeFunctionOnNextCall(opt_function)\n"
-    "\n"
-    "test(10, 10);\n"
-    "\n"
-    "test(10, 0);\n"
+    "for (var i = 0; i < 10; ++i) test(undefined);\n"
     "\n"
     "stopProfiling();\n"
     "\n";
@@ -1768,8 +1760,5 @@ TEST(CollectDeoptEvents) {
   const v8::CpuProfileNode* opt_function = GetSimpleBranch(
       env->GetIsolate(), profile->GetTopDownRoot(), branch, arraysize(branch));
   CHECK(opt_function);
-  const i::ProfileNode* iopt_function =
-      reinterpret_cast<const i::ProfileNode*>(opt_function);
-  CHECK_EQ(2, iopt_function->deopt_infos().length());
   iprofiler->DeleteProfile(iprofile);
 }