Skip to content

Commit 6159ef6

Browse files
deps: V8: backport d259a9e2ead7
Original commit message: [maglev] Add source position collection This change adds support for collecting source positions in Maglev for profiling. It wires a SourcePositionTableBuilder through the code generator and uses the existing source positions from the graph labeller. Fixed: 436575053 Change-Id: I64fa1e8829b036e498fa1d756c996716d16fb2a8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6897146 Reviewed-by: Victor Gomes <victorgomes@chromium.org> Auto-Submit: Leszek Swirski <leszeks@chromium.org> Commit-Queue: Leszek Swirski <leszeks@chromium.org> Cr-Commit-Position: refs/heads/main@{#102119} Before this change Maglev attached an empty source position table to its generated Code objects (set_empty_source_position_table()). As a result, when profiling with `--perf-prof --interpreted-frames-native-stack` (linux perf / samply), Maglev-optimized frames carried no line/inlining information in the jitdump, so inlined call chains were lost in the resulting flamegraphs (baseline and TurboFan code were unaffected). This backport restores per-pc source positions for Maglev code, matching V8 >= 15.x and newer Node.js. The change is gated on collect_source_positions() (enabled by --detailed-line-info / profiling); the steady-state generated machine code is unchanged and there is no extra cost when not profiling. Adaptations for V8 13.6 (node): the graph-labeller creation in MaglevCompiler::Compile is added as a separate condition because node's 13.6 tree creates the labeller in a differently-shaped block than upstream; the rest is a direct cherry-pick. Refs: v8/v8@d259a9e Fixes: #63816
1 parent 8484306 commit 6159ef6

6 files changed

Lines changed: 48 additions & 5 deletions

File tree

common.gypi

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
# Reset this number to 0 on major V8 upgrades.
4040
# Increment by one for each non-official patch applied to deps/v8.
41-
'v8_embedder_string': '-node.48',
41+
'v8_embedder_string': '-node.49',
4242

4343
##### V8 defaults for Node.js #####
4444

deps/v8/src/maglev/maglev-code-gen-state.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "src/codegen/label.h"
1010
#include "src/codegen/machine-type.h"
1111
#include "src/codegen/maglev-safepoint-table.h"
12+
#include "src/codegen/source-position-table.h"
1213
#include "src/common/globals.h"
1314
#include "src/compiler/backend/instruction.h"
1415
#include "src/compiler/js-heap-broker.h"
@@ -34,9 +35,11 @@ class MaglevCodeGenState {
3435
public:
3536
MaglevCodeGenState(MaglevCompilationInfo* compilation_info,
3637
MaglevSafepointTableBuilder* safepoint_table_builder,
38+
SourcePositionTableBuilder* source_position_table_builder,
3739
uint32_t max_block_id)
3840
: compilation_info_(compilation_info),
3941
safepoint_table_builder_(safepoint_table_builder),
42+
source_position_table_builder_(source_position_table_builder),
4043
real_jump_target_(max_block_id) {}
4144

4245
void set_tagged_slots(int slots) { tagged_slots_ = slots; }
@@ -80,6 +83,9 @@ class MaglevCodeGenState {
8083
MaglevSafepointTableBuilder* safepoint_table_builder() const {
8184
return safepoint_table_builder_;
8285
}
86+
SourcePositionTableBuilder* source_position_table_builder() const {
87+
return source_position_table_builder_;
88+
}
8389
MaglevCompilationInfo* compilation_info() const { return compilation_info_; }
8490

8591
Label* entry_label() { return &entry_label_; }
@@ -121,6 +127,7 @@ class MaglevCodeGenState {
121127
private:
122128
MaglevCompilationInfo* const compilation_info_;
123129
MaglevSafepointTableBuilder* const safepoint_table_builder_;
130+
SourcePositionTableBuilder* const source_position_table_builder_;
124131

125132
std::vector<DeferredCodeInfo*> deferred_code_;
126133
std::vector<EagerDeoptInfo*> eager_deopts_;

deps/v8/src/maglev/maglev-code-generator.cc

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -707,7 +707,14 @@ class ExceptionHandlerTrampolineBuilder {
707707
class MaglevCodeGeneratingNodeProcessor {
708708
public:
709709
MaglevCodeGeneratingNodeProcessor(MaglevAssembler* masm, Zone* zone)
710-
: masm_(masm), zone_(zone) {}
710+
: masm_(masm),
711+
zone_(zone),
712+
// Cache for faster check.
713+
collect_source_positions_(masm->code_gen_state()
714+
->compilation_info()
715+
->collect_source_positions()) {
716+
DCHECK_IMPLIES(collect_source_positions_, graph_labeller() != nullptr);
717+
}
711718

712719
void PreProcessGraph(Graph* graph) {
713720
// TODO(victorgomes): I wonder if we want to create a struct that shares
@@ -800,6 +807,15 @@ class MaglevCodeGeneratingNodeProcessor {
800807
<< PrintNode(graph_labeller(), node);
801808
__ RecordComment(ss.str());
802809
}
810+
if (collect_source_positions_) {
811+
// TODO(leszeks): Consider collecting source position in a more memory
812+
// friendly way, if we don't need the whole graph labeller.
813+
const auto& provenance = graph_labeller()->GetNodeProvenance(node);
814+
if (provenance.position.IsKnown()) {
815+
code_gen_state()->source_position_table_builder()->AddPosition(
816+
masm_->pc_offset(), provenance.position, false);
817+
}
818+
}
803819

804820
if (v8_flags.maglev_assert_stack_size) {
805821
__ AssertStackSizeCorrect();
@@ -1105,6 +1121,7 @@ class MaglevCodeGeneratingNodeProcessor {
11051121
}
11061122
MaglevAssembler* const masm_;
11071123
Zone* zone_;
1124+
bool collect_source_positions_;
11081125
};
11091126

11101127
class SafepointingNodeProcessor {
@@ -1695,8 +1712,9 @@ MaglevCodeGenerator::MaglevCodeGenerator(
16951712
safepoint_table_builder_(compilation_info->zone(),
16961713
graph->tagged_stack_slots()),
16971714
frame_translation_builder_(compilation_info->zone()),
1715+
source_position_table_builder_(compilation_info->zone()),
16981716
code_gen_state_(compilation_info, &safepoint_table_builder_,
1699-
graph->max_block_id()),
1717+
&source_position_table_builder_, graph->max_block_id()),
17001718
masm_(isolate->GetMainThreadIsolateUnsafe(), compilation_info->zone(),
17011719
&code_gen_state_),
17021720
graph_(graph),
@@ -1919,6 +1937,10 @@ MaybeHandle<Code> MaglevCodeGenerator::BuildCodeObject(
19191937
LocalIsolate* local_isolate) {
19201938
if (!code_gen_succeeded_) return {};
19211939

1940+
// Allocate the source position table.
1941+
Handle<TrustedByteArray> source_positions =
1942+
source_position_table_builder_.ToSourcePositionTable(local_isolate);
1943+
19221944
Handle<DeoptimizationData> deopt_data =
19231945
(v8_flags.maglev_deopt_data_on_background &&
19241946
!v8_flags.maglev_build_code_on_background)
@@ -1934,7 +1956,7 @@ MaybeHandle<Code> MaglevCodeGenerator::BuildCodeObject(
19341956
.set_stack_slots(stack_slot_count_with_fixed_frame())
19351957
.set_parameter_count(parameter_count())
19361958
.set_deoptimization_data(deopt_data)
1937-
.set_empty_source_position_table()
1959+
.set_source_position_table(source_positions)
19381960
.set_inlined_bytecode_size(graph_->total_inlined_bytecode_size())
19391961
.set_osr_offset(
19401962
code_gen_state_.compilation_info()->toplevel_osr_offset());

deps/v8/src/maglev/maglev-code-generator.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#define V8_MAGLEV_MAGLEV_CODE_GENERATOR_H_
77

88
#include "src/codegen/maglev-safepoint-table.h"
9+
#include "src/codegen/source-position-table.h"
910
#include "src/common/globals.h"
1011
#include "src/deoptimizer/frame-translation-builder.h"
1112
#include "src/maglev/maglev-assembler.h"
@@ -54,6 +55,7 @@ class MaglevCodeGenerator final {
5455
LocalIsolate* local_isolate_;
5556
MaglevSafepointTableBuilder safepoint_table_builder_;
5657
FrameTranslationBuilder frame_translation_builder_;
58+
SourcePositionTableBuilder source_position_table_builder_;
5759
MaglevCodeGenState code_gen_state_;
5860
MaglevAssembler masm_;
5961
Graph* const graph_;

deps/v8/src/maglev/maglev-compiler.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,18 @@ bool MaglevCompiler::Compile(LocalIsolate* local_isolate,
8282
compilation_info->set_graph_labeller(new MaglevGraphLabeller());
8383
}
8484

85+
// When collecting source positions for profiling, create a graph labeller
86+
// so per-node provenance (including inlined positions) is recorded during
87+
// graph building and a real source position table can be attached to the
88+
// Code object.
89+
// Upstream V8 folds this into the single graph-labeller condition; node's
90+
// V8 creates the labeller in a differently-shaped block, so the condition
91+
// is added separately here.
92+
if (compilation_info->collect_source_positions() &&
93+
!compilation_info->has_graph_labeller()) {
94+
compilation_info->set_graph_labeller(new MaglevGraphLabeller());
95+
}
96+
8597
if (is_tracing_enabled &&
8698
(v8_flags.print_maglev_code || v8_flags.print_maglev_graph ||
8799
v8_flags.print_maglev_graphs || v8_flags.trace_maglev_graph_building ||

deps/v8/test/unittests/maglev/maglev-assembler-unittest.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ class MaglevAssemblerTest : public MaglevTest {
1818
public:
1919
MaglevAssemblerTest()
2020
: MaglevTest(),
21-
codegen_state(nullptr, nullptr, 0),
21+
codegen_state(nullptr, nullptr, nullptr, 0),
2222
as(isolate(), zone(), &codegen_state) {}
2323

2424
void FinalizeAndRun(Label* pass, Label* fail) {

0 commit comments

Comments
 (0)