From 4feb5ce7fd5ef8c933f3f5dff2eca1173f85c1e9 Mon Sep 17 00:00:00 2001 From: Peter Marshall Date: Wed, 11 Apr 2018 15:09:15 +0200 Subject: [PATCH] [cpu-profiler] Fix bugs and add tests for JITLineInfoTable Looking up line numbers with the JITLineInfoTable would sometimes give wrong answers. Fix these bugs and add a cctest for this data structure. Also do some cleanup while we're here like inlining the (empty) constructor and destructor and removing the empty() method which is only used unnecessarily anyway, to make the contract of GetSourceLineNumber a bit clearer. Also rename the data structure to SourcePositionTable, because it doesn't just provide info for JIT code, but also bytecode, and 'Info' is pretty ambiguous. Bug: v8:7018 Change-Id: I126581c844d85df6b2b3f80f2f5acbce01c16ba1 Reviewed-on: https://chromium-review.googlesource.com/1006795 Reviewed-by: Yang Guo Commit-Queue: Peter Marshall Cr-Commit-Position: refs/heads/master@{#52571} --- src/profiler/profile-generator-inl.h | 2 +- src/profiler/profile-generator.cc | 28 ++++++--------- src/profiler/profile-generator.h | 20 +++++------ src/profiler/profiler-listener.cc | 6 ++-- src/profiler/profiler-listener.h | 2 +- test/cctest/test-cpu-profiler.cc | 54 ++++++++++++++++++++++++++-- 6 files changed, 77 insertions(+), 35 deletions(-) diff --git a/src/profiler/profile-generator-inl.h b/src/profiler/profile-generator-inl.h index 970d462937..e5154a75b9 100644 --- a/src/profiler/profile-generator-inl.h +++ b/src/profiler/profile-generator-inl.h @@ -13,7 +13,7 @@ namespace internal { CodeEntry::CodeEntry(CodeEventListener::LogEventsAndTags tag, const char* name, const char* name_prefix, const char* resource_name, int line_number, int column_number, - std::unique_ptr line_info, + std::unique_ptr line_info, Address instruction_start) : bit_field_(TagField::encode(tag) | BuiltinIdField::encode(Builtins::builtin_count)), diff --git a/src/profiler/profile-generator.cc b/src/profiler/profile-generator.cc index db95718936..4deb40c30a 100644 --- a/src/profiler/profile-generator.cc +++ b/src/profiler/profile-generator.cc @@ -18,27 +18,23 @@ namespace v8 { namespace internal { - -JITLineInfoTable::JITLineInfoTable() {} - - -JITLineInfoTable::~JITLineInfoTable() {} - - -void JITLineInfoTable::SetPosition(int pc_offset, int line) { +void SourcePositionTable::SetPosition(int pc_offset, int line) { DCHECK_GE(pc_offset, 0); DCHECK_GT(line, 0); // The 1-based number of the source line. if (GetSourceLineNumber(pc_offset) != line) { - pc_offset_map_.insert(std::make_pair(pc_offset, line)); + auto result = pc_offset_map_.insert(std::make_pair(pc_offset, line)); + // Check that a new element was inserted. + USE(result); + DCHECK(result.second); } } +int SourcePositionTable::GetSourceLineNumber(int pc_offset) const { + if (pc_offset_map_.empty()) return v8::CpuProfileNode::kNoLineNumberInfo; -int JITLineInfoTable::GetSourceLineNumber(int pc_offset) const { - PcOffsetMap::const_iterator it = pc_offset_map_.lower_bound(pc_offset); - if (it == pc_offset_map_.end()) { - if (pc_offset_map_.empty()) return v8::CpuProfileNode::kNoLineNumberInfo; - return (--pc_offset_map_.end())->second; + PcOffsetMap::const_iterator it = pc_offset_map_.upper_bound(pc_offset); + if (it != pc_offset_map_.begin()) { + return (--it)->second; } return it->second; } @@ -121,9 +117,7 @@ void CodeEntry::SetBuiltinId(Builtins::Name id) { int CodeEntry::GetSourceLine(int pc_offset) const { - if (line_info_ && !line_info_->empty()) { - return line_info_->GetSourceLineNumber(pc_offset); - } + if (line_info_) return line_info_->GetSourceLineNumber(pc_offset); return v8::CpuProfileNode::kNoLineNumberInfo; } diff --git a/src/profiler/profile-generator.h b/src/profiler/profile-generator.h index 5abb955a46..a9ba42f97c 100644 --- a/src/profiler/profile-generator.h +++ b/src/profiler/profile-generator.h @@ -20,23 +20,21 @@ namespace internal { struct TickSample; -// Provides a mapping from the offsets within generated code to -// the source line. -class JITLineInfoTable : public Malloced { +// Provides a mapping from the offsets within generated code or a bytecode array +// to the source line. +class SourcePositionTable : public Malloced { public: - JITLineInfoTable(); - ~JITLineInfoTable(); + SourcePositionTable() {} + ~SourcePositionTable() {} void SetPosition(int pc_offset, int line); int GetSourceLineNumber(int pc_offset) const; - bool empty() const { return pc_offset_map_.empty(); } - private: // pc_offset -> source line typedef std::map PcOffsetMap; PcOffsetMap pc_offset_map_; - DISALLOW_COPY_AND_ASSIGN(JITLineInfoTable); + DISALLOW_COPY_AND_ASSIGN(SourcePositionTable); }; @@ -48,7 +46,7 @@ class CodeEntry { const char* resource_name = CodeEntry::kEmptyResourceName, int line_number = v8::CpuProfileNode::kNoLineNumberInfo, int column_number = v8::CpuProfileNode::kNoColumnNumberInfo, - std::unique_ptr line_info = nullptr, + std::unique_ptr line_info = nullptr, Address instruction_start = nullptr); const char* name_prefix() const { return name_prefix_; } @@ -57,7 +55,7 @@ class CodeEntry { const char* resource_name() const { return resource_name_; } int line_number() const { return line_number_; } int column_number() const { return column_number_; } - const JITLineInfoTable* line_info() const { return line_info_.get(); } + const SourcePositionTable* line_info() const { return line_info_.get(); } int script_id() const { return script_id_; } void set_script_id(int script_id) { script_id_ = script_id; } int position() const { return position_; } @@ -162,7 +160,7 @@ class CodeEntry { const char* bailout_reason_; const char* deopt_reason_; int deopt_id_; - std::unique_ptr line_info_; + std::unique_ptr line_info_; Address instruction_start_; // Should be an unordered_map, but it doesn't currently work on Win & MacOS. std::map>> inline_locations_; diff --git a/src/profiler/profiler-listener.cc b/src/profiler/profiler-listener.cc index 16ec9d883a..df18a5c836 100644 --- a/src/profiler/profiler-listener.cc +++ b/src/profiler/profiler-listener.cc @@ -83,10 +83,10 @@ void ProfilerListener::CodeCreateEvent(CodeEventListener::LogEventsAndTags tag, CodeEventsContainer evt_rec(CodeEventRecord::CODE_CREATION); CodeCreateEventRecord* rec = &evt_rec.CodeCreateEventRecord_; rec->start = abstract_code->address(); - std::unique_ptr line_table; + std::unique_ptr line_table; if (shared->script()->IsScript()) { Script* script = Script::cast(shared->script()); - line_table.reset(new JITLineInfoTable()); + line_table.reset(new SourcePositionTable()); int offset = abstract_code->IsCode() ? Code::kHeaderSize : BytecodeArray::kHeaderSize; for (SourcePositionTableIterator it(abstract_code->source_position_table()); @@ -296,7 +296,7 @@ void ProfilerListener::RecordDeoptInlinedFrames(CodeEntry* entry, CodeEntry* ProfilerListener::NewCodeEntry( CodeEventListener::LogEventsAndTags tag, const char* name, const char* name_prefix, const char* resource_name, int line_number, - int column_number, std::unique_ptr line_info, + int column_number, std::unique_ptr line_info, Address instruction_start) { std::unique_ptr code_entry = base::make_unique( tag, name, name_prefix, resource_name, line_number, column_number, diff --git a/src/profiler/profiler-listener.h b/src/profiler/profiler-listener.h index 791b66f3ed..235a8ed671 100644 --- a/src/profiler/profiler-listener.h +++ b/src/profiler/profiler-listener.h @@ -58,7 +58,7 @@ class ProfilerListener : public CodeEventListener { const char* resource_name = CodeEntry::kEmptyResourceName, int line_number = v8::CpuProfileNode::kNoLineNumberInfo, int column_number = v8::CpuProfileNode::kNoColumnNumberInfo, - std::unique_ptr line_info = nullptr, + std::unique_ptr line_info = nullptr, Address instruction_start = nullptr); void AddObserver(CodeEventObserver* observer); diff --git a/test/cctest/test-cpu-profiler.cc b/test/cctest/test-cpu-profiler.cc index a64ea9c307..29276c298e 100644 --- a/test/cctest/test-cpu-profiler.cc +++ b/test/cctest/test-cpu-profiler.cc @@ -27,6 +27,9 @@ // // Tests of profiles generator and utilities. +#include +#include + #include "src/v8.h" #include "include/v8-profiler.h" @@ -1108,9 +1111,10 @@ static void TickLines(bool optimize) { CodeEntry* func_entry = generator->code_map()->FindEntry(code_address); CHECK(func_entry); CHECK_EQ(0, strcmp(func_name, func_entry->name())); - const i::JITLineInfoTable* line_info = func_entry->line_info(); + const i::SourcePositionTable* line_info = func_entry->line_info(); CHECK(line_info); - CHECK(!line_info->empty()); + CHECK_NE(v8::CpuProfileNode::kNoLineNumberInfo, + line_info->GetSourceLineNumber(100)); // Check the hit source lines using V8 Public APIs. const i::ProfileTree* tree = profile->top_down(); @@ -2433,6 +2437,52 @@ TEST(NativeFrameStackTrace) { profile->Delete(); } +TEST(SourcePositionTable) { + std::unique_ptr info(new SourcePositionTable()); + + // Newly created tables should return NoLineNumberInfo for any lookup. + int no_info = v8::CpuProfileNode::kNoLineNumberInfo; + CHECK_EQ(no_info, info->GetSourceLineNumber(std::numeric_limits::min())); + CHECK_EQ(no_info, info->GetSourceLineNumber(0)); + CHECK_EQ(no_info, info->GetSourceLineNumber(1)); + CHECK_EQ(no_info, info->GetSourceLineNumber(9)); + CHECK_EQ(no_info, info->GetSourceLineNumber(10)); + CHECK_EQ(no_info, info->GetSourceLineNumber(11)); + CHECK_EQ(no_info, info->GetSourceLineNumber(19)); + CHECK_EQ(no_info, info->GetSourceLineNumber(20)); + CHECK_EQ(no_info, info->GetSourceLineNumber(21)); + CHECK_EQ(no_info, info->GetSourceLineNumber(100)); + CHECK_EQ(no_info, info->GetSourceLineNumber(std::numeric_limits::max())); + + info->SetPosition(10, 1); + info->SetPosition(20, 2); + + // The only valid return values are 1 or 2 - every pc maps to a line number. + CHECK_EQ(1, info->GetSourceLineNumber(std::numeric_limits::min())); + CHECK_EQ(1, info->GetSourceLineNumber(0)); + CHECK_EQ(1, info->GetSourceLineNumber(1)); + CHECK_EQ(1, info->GetSourceLineNumber(9)); + CHECK_EQ(1, info->GetSourceLineNumber(10)); + CHECK_EQ(1, info->GetSourceLineNumber(11)); + CHECK_EQ(1, info->GetSourceLineNumber(19)); + CHECK_EQ(2, info->GetSourceLineNumber(20)); + CHECK_EQ(2, info->GetSourceLineNumber(21)); + CHECK_EQ(2, info->GetSourceLineNumber(100)); + CHECK_EQ(2, info->GetSourceLineNumber(std::numeric_limits::max())); + + // Test SetPosition behavior. + // Setting a position to the same value has no effect. + info->SetPosition(10, 1); + CHECK_EQ(1, info->GetSourceLineNumber(9)); + CHECK_EQ(1, info->GetSourceLineNumber(10)); + CHECK_EQ(1, info->GetSourceLineNumber(11)); + + info->SetPosition(25, 3); + CHECK_EQ(2, info->GetSourceLineNumber(21)); + CHECK_EQ(3, info->GetSourceLineNumber(100)); + CHECK_EQ(3, info->GetSourceLineNumber(std::numeric_limits::max())); +} + } // namespace test_cpu_profiler } // namespace internal } // namespace v8