[cpu-profiler] use new source position information for deoptimization in cpu profiler

src/profiler/profiler-listener.cc

Index: src/profiler/profiler-listener.cc
diff --git a/src/profiler/profiler-listener.cc b/src/profiler/profiler-listener.cc
index ea85acfea9ebca6850353b190744d925d5596154..517adfa5cf3b01ee74badbb346d8d8d681bf3966 100644
--- a/src/profiler/profiler-listener.cc
+++ b/src/profiler/profiler-listener.cc
@@ -94,14 +94,9 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag,
     int end_position = shared->end_position();
     for (SourcePositionTableIterator it(abstract_code->source_position_table());
          !it.done(); it.Advance()) {
+      if (it.source_position().InliningId() != SourcePosition::kNotInlined)

[alph, 2016/11/17 00:09:44]

Why do you skip inlined positions?
If it's not ready for this patch could you please keep the TODO.

[Tobias Tebbi, 2016/11/17 11:41:34]

Sorry, yes, the TODO should stay. I skip inlined positions to maintain the old
behavior. Is it safe to add inlined positions here as long as they are in the
same script? We can easily get all needed information here, including the
inlining stack and script ids. I just don't understand the cpu profiler well
enough to know what to do here.

+        continue;
       int position = it.source_position().ScriptOffset();
-      // TODO(alph): in case of inlining the position may correspond to an
-      // inlined function source code. Do not collect positions that fall
-      // beyond the function source code. There's however a chance the
-      // inlined function has similar positions but in another script. So
-      // the proper fix is to store script_id in some form along with the
-      // inlined function positions.
-      if (position < start_position || position >= end_position) continue;
       int line_number = script->GetLineNumber(position) + 1;
       int pc_offset = it.code_offset() + offset;
       line_table->SetPosition(pc_offset, line_number);
@@ -156,7 +151,6 @@ void ProfilerListener::CodeDeoptEvent(Code* code, Address pc,
   Deoptimizer::DeoptInfo info = Deoptimizer::GetDeoptInfo(code, pc);
   rec->start = code->address();
   rec->deopt_reason = DeoptimizeReasonToString(info.deopt_reason);
-  rec->position = info.position;
   rec->deopt_id = info.deopt_id;
   rec->pc = reinterpret_cast<void*>(pc);
   rec->fp_to_sp_delta = fp_to_sp_delta;
@@ -254,55 +248,35 @@ void ProfilerListener::RecordInliningInfo(CodeEntry* entry,
 void ProfilerListener::RecordDeoptInlinedFrames(CodeEntry* entry,
                                                 AbstractCode* abstract_code) {
   if (abstract_code->kind() != AbstractCode::OPTIMIZED_FUNCTION) return;
-  Code* code = abstract_code->GetCode();
-  DeoptimizationInputData* deopt_input_data =
-      DeoptimizationInputData::cast(code->deoptimization_data());
-  int const mask = RelocInfo::ModeMask(RelocInfo::DEOPT_ID);
-  for (RelocIterator rit(code, mask); !rit.done(); rit.next()) {
-    RelocInfo* reloc_info = rit.rinfo();
-    DCHECK(RelocInfo::IsDeoptId(reloc_info->rmode()));
-    int deopt_id = static_cast<int>(reloc_info->data());
-    int translation_index =
-        deopt_input_data->TranslationIndex(deopt_id)->value();
-    TranslationIterator it(deopt_input_data->TranslationByteArray(),
-                           translation_index);
-    Translation::Opcode opcode = static_cast<Translation::Opcode>(it.Next());
-    DCHECK_EQ(Translation::BEGIN, opcode);
-    it.Skip(Translation::NumberOfOperandsFor(opcode));
-    std::vector<CodeEntry::DeoptInlinedFrame> inlined_frames;
-    while (it.HasNext() &&
-           Translation::BEGIN !=
-               (opcode = static_cast<Translation::Opcode>(it.Next()))) {
-      if (opcode != Translation::JS_FRAME &&
-          opcode != Translation::INTERPRETED_FRAME) {
-        it.Skip(Translation::NumberOfOperandsFor(opcode));
-        continue;
-      }
-      BailoutId ast_id = BailoutId(it.Next());
-      int shared_info_id = it.Next();
-      it.Next();  // Skip height
-      SharedFunctionInfo* shared = SharedFunctionInfo::cast(
-          deopt_input_data->LiteralArray()->get(shared_info_id));
-      int source_position;
-      if (opcode == Translation::INTERPRETED_FRAME) {
-        source_position =
-            Deoptimizer::ComputeSourcePositionFromBytecodeArray(shared, ast_id);
-      } else {
-        DCHECK(opcode == Translation::JS_FRAME);
-        source_position =
-            Deoptimizer::ComputeSourcePositionFromBaselineCode(shared, ast_id);
+  Handle<Code> code(abstract_code->GetCode());

[alph, 2016/11/17 00:09:44]

Why it needs a handle?

[Tobias Tebbi, 2016/11/17 11:41:34]

It needs a handle because last_position.InliningStack(code) is handlified (the
reason is that it might initialize line ends, which allocates). Is there a
reason why it should stay unhandlified? Is it safe to allocate from the profile
listener?

[alph, 2016/11/18 22:51:46]

Makes sense. Thanks. Handles are slower than raw access, so I'd avoid them
whenever possible.

+
+  SourcePosition last_position = SourcePosition::Unknown();
+  int mask = RelocInfo::ModeMask(RelocInfo::DEOPT_ID) |
+             RelocInfo::ModeMask(RelocInfo::DEOPT_SCRIPT_OFFSET) |
+             RelocInfo::ModeMask(RelocInfo::DEOPT_INLINING_ID);
+  for (RelocIterator it(*code, mask); !it.done(); it.next()) {
+    RelocInfo* info = it.rinfo();
+    if (info->rmode() == RelocInfo::DEOPT_SCRIPT_OFFSET) {
+      int script_offset = static_cast<int>(info->data());
+      it.next();
+      DCHECK(it.rinfo()->rmode() == RelocInfo::DEOPT_INLINING_ID);
+      int inlining_id = static_cast<int>(it.rinfo()->data());
+      last_position = SourcePosition(script_offset, inlining_id);

[alph, 2016/11/17 00:09:44]

nit: use continue

+    } else if (info->rmode() == RelocInfo::DEOPT_ID) {
+      int deopt_id = static_cast<int>(info->data());
+      if (!last_position.IsKnown()) continue;
+      std::vector<CpuProfileDeoptFrame> inlined_frames;
+      for (SourcePositionInfo& pos_info : last_position.InliningStack(code)) {
+        DCHECK(pos_info.position.ScriptOffset() != kNoSourcePosition);
+        size_t offset = static_cast<size_t>(pos_info.position.ScriptOffset());
+        int script_id = Script::cast(pos_info.function->script())->id();
+        inlined_frames.push_back(CpuProfileDeoptFrame({script_id, offset}));
       }
-      int script_id = v8::UnboundScript::kNoScriptId;
-      if (shared->script()->IsScript()) {
-        Script* script = Script::cast(shared->script());
-        script_id = script->id();
+      if (!inlined_frames.empty() &&
+          !entry->HasDeoptInlinedFramesFor(deopt_id)) {
+        entry->AddDeoptInlinedFrames(deopt_id, inlined_frames);
+        DCHECK(inlined_frames.empty());
       }
-      CodeEntry::DeoptInlinedFrame frame = {source_position, script_id};
-      inlined_frames.push_back(frame);
-    }
-    if (!inlined_frames.empty() && !entry->HasDeoptInlinedFramesFor(deopt_id)) {
-      entry->AddDeoptInlinedFrames(deopt_id, inlined_frames);
-      DCHECK(inlined_frames.empty());
     }
   }
 }