Skip to content

Commit 058870c

Browse files
jdapenaV8 LUCI CQ
jdapena
authored and
V8 LUCI CQ
committed
[heap] Fix allocation tracker line end retrieval
Tests in NodeJS with DCHECK enabled were failing because of two different problems: - One was that we were also disallowing heap allocations in the serialization stage. But NodeJS tests process the heap snapshot result in JS, so all those were broken. - But, also, the code that would retrieve from line ends would assume all the scripts populated line ends in the snapshot. But this was wrong. To fix it, this patch adds another storage of the line ends in the allocation tracker. This storage needs to keep weak references to the scripts so we do not leak the line ends data when scripts are disposed. This change keeps a weak reference to the scripts so, when they are freed, the line ends cache is also freed as it is not useful anymore. Node issue: nodejs/node-v8#282 Change-Id: I4314d707903175f0005893e9aa06d3ae52fc57f8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5531355 Commit-Queue: José Dapena Paz <[email protected]> Reviewed-by: Michael Lippautz <[email protected]> Reviewed-by: Simon Zünd <[email protected]> Cr-Commit-Position: refs/heads/main@{#94418}
1 parent a0a4172 commit 058870c

3 files changed

Lines changed: 94 additions & 15 deletions

File tree

‎src/profiler/allocation-tracker.cc‎

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@
44

55
#include "src/profiler/allocation-tracker.h"
66

7+
#include "src/api/api-inl.h"
8+
#include "src/api/api.h"
79
#include "src/execution/frames-inl.h"
810
#include "src/handles/global-handles-inl.h"
911
#include "src/objects/objects-inl.h"
1012
#include "src/profiler/heap-snapshot-generator-inl.h"
13+
#include "src/utils/utils.h"
1114

1215
namespace v8 {
1316
namespace internal {
@@ -96,7 +99,9 @@ AllocationTracker::FunctionInfo::FunctionInfo()
9699
function_id(0),
97100
script_name(""),
98101
script_id(0),
99-
start_position(-1) {}
102+
start_position(-1),
103+
line(-1),
104+
column(-1) {}
100105

101106
void AddressToTraceMap::AddRange(Address start, int size,
102107
unsigned trace_node_id) {
@@ -199,7 +204,7 @@ void AllocationTracker::AllocationEvent(Address addr, int size) {
199204
SnapshotObjectId id =
200205
ids_->FindOrAddEntry(shared.address(), shared->Size(),
201206
HeapObjectsMap::MarkEntryAccessed::kNo);
202-
allocation_trace_buffer_[length++] = AddFunctionInfo(shared, id);
207+
allocation_trace_buffer_[length++] = AddFunctionInfo(shared, id, isolate);
203208
it.Advance();
204209
}
205210
if (length == 0) {
@@ -220,8 +225,60 @@ static uint32_t SnapshotObjectIdHash(SnapshotObjectId id) {
220225
return ComputeUnseededHash(static_cast<uint32_t>(id));
221226
}
222227

228+
AllocationTracker::ScriptData::ScriptData(Tagged<Script> script,
229+
Isolate* isolate,
230+
AllocationTracker* tracker)
231+
: script_id_(script->id()),
232+
line_ends_(Script::GetLineEnds(isolate, handle(script, isolate))),
233+
tracker_(tracker) {
234+
DirectHandle<Script> script_direct_handle(script, isolate);
235+
auto local_script = ToApiHandle<debug::Script>(script_direct_handle, isolate);
236+
script_.Reset(local_script->GetIsolate(), local_script);
237+
script_.SetWeak(this, &HandleWeakScript, v8::WeakCallbackType::kParameter);
238+
}
239+
240+
AllocationTracker::ScriptData::~ScriptData() {
241+
if (!script_.IsEmpty()) {
242+
script_.ClearWeak();
243+
}
244+
}
245+
246+
void AllocationTracker::ScriptData::HandleWeakScript(
247+
const v8::WeakCallbackInfo<ScriptData>& data) {
248+
ScriptData* script_data = reinterpret_cast<ScriptData*>(data.GetParameter());
249+
script_data->script_.ClearWeak();
250+
script_data->script_.Reset();
251+
script_data->tracker_->scripts_data_map_.erase(script_data->script_id_);
252+
}
253+
254+
String::LineEndsVector& AllocationTracker::GetOrCreateLineEnds(
255+
Tagged<Script> script, Isolate* isolate) {
256+
auto it = scripts_data_map_.find(script->id());
257+
if (it == scripts_data_map_.end()) {
258+
auto inserted =
259+
scripts_data_map_.try_emplace(script->id(), script, isolate, this);
260+
CHECK(inserted.second);
261+
return inserted.first->second.line_ends();
262+
} else {
263+
return it->second.line_ends();
264+
}
265+
}
266+
267+
Script::PositionInfo AllocationTracker::GetScriptPositionInfo(
268+
Tagged<Script> script, Isolate* isolate, int start) {
269+
Script::PositionInfo position_info;
270+
if (script->has_line_ends()) {
271+
script->GetPositionInfo(start, &position_info);
272+
} else {
273+
script->GetPositionInfoWithLineEnds(start, &position_info,
274+
GetOrCreateLineEnds(script, isolate));
275+
}
276+
return position_info;
277+
}
278+
223279
unsigned AllocationTracker::AddFunctionInfo(Tagged<SharedFunctionInfo> shared,
224-
SnapshotObjectId id) {
280+
SnapshotObjectId id,
281+
Isolate* isolate) {
225282
base::HashMap::Entry* entry = id_to_function_info_index_.LookupOrInsert(
226283
reinterpret_cast<void*>(id), SnapshotObjectIdHash(id));
227284
if (entry->value == nullptr) {
@@ -236,6 +293,10 @@ unsigned AllocationTracker::AddFunctionInfo(Tagged<SharedFunctionInfo> shared,
236293
}
237294
info->script_id = script->id();
238295
info->start_position = shared->StartPosition();
296+
Script::PositionInfo position_info =
297+
GetScriptPositionInfo(script, isolate, info->start_position);
298+
info->line = position_info.line;
299+
info->column = position_info.column;
239300
}
240301
entry->value = reinterpret_cast<void*>(function_info_list_.size());
241302
function_info_list_.push_back(info);

‎src/profiler/allocation-tracker.h‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,18 @@
66
#define V8_PROFILER_ALLOCATION_TRACKER_H_
77

88
#include <map>
9+
#include <unordered_map>
910
#include <vector>
1011

1112
#include "include/v8-persistent-handle.h"
1213
#include "include/v8-profiler.h"
1314
#include "include/v8-unwinder.h"
1415
#include "src/base/hashmap.h"
1516
#include "src/base/vector.h"
17+
#include "src/debug/debug-interface.h"
1618
#include "src/handles/handles.h"
19+
#include "src/objects/script.h"
20+
#include "src/objects/string.h"
1721

1822
namespace v8 {
1923
namespace internal {
@@ -105,6 +109,8 @@ class AllocationTracker {
105109
const char* script_name;
106110
int script_id;
107111
int start_position;
112+
int line;
113+
int column;
108114
};
109115

110116
AllocationTracker(HeapObjectsMap* ids, StringsStorage* names);
@@ -121,8 +127,12 @@ class AllocationTracker {
121127
AddressToTraceMap* address_to_trace() { return &address_to_trace_; }
122128

123129
private:
124-
unsigned AddFunctionInfo(Tagged<SharedFunctionInfo> info,
125-
SnapshotObjectId id);
130+
unsigned AddFunctionInfo(Tagged<SharedFunctionInfo> info, SnapshotObjectId id,
131+
Isolate* isolate);
132+
String::LineEndsVector& GetOrCreateLineEnds(Tagged<Script> script,
133+
Isolate* isolate);
134+
Script::PositionInfo GetScriptPositionInfo(Tagged<Script> script,
135+
Isolate* isolate, int start);
126136
unsigned functionInfoIndexForVMState(StateTag state);
127137

128138
static const int kMaxAllocationTraceLength = 64;
@@ -134,6 +144,22 @@ class AllocationTracker {
134144
base::HashMap id_to_function_info_index_;
135145
unsigned info_index_for_other_state_;
136146
AddressToTraceMap address_to_trace_;
147+
using ScriptId = int;
148+
class ScriptData {
149+
public:
150+
ScriptData(Tagged<Script>, Isolate*, AllocationTracker*);
151+
~ScriptData();
152+
String::LineEndsVector& line_ends() { return line_ends_; }
153+
154+
private:
155+
static void HandleWeakScript(const v8::WeakCallbackInfo<ScriptData>&);
156+
Global<debug::Script> script_;
157+
ScriptId script_id_;
158+
String::LineEndsVector line_ends_;
159+
AllocationTracker* tracker_;
160+
};
161+
using ScriptsDataMap = std::unordered_map<ScriptId, ScriptData>;
162+
ScriptsDataMap scripts_data_map_;
137163
};
138164

139165
} // namespace internal

‎src/profiler/heap-snapshot-generator.cc‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3090,7 +3090,6 @@ bool HeapSnapshotGenerator::FillReferences() {
30903090
const int HeapSnapshotJSONSerializer::kNodeFieldsCount = 7;
30913091

30923092
void HeapSnapshotJSONSerializer::Serialize(v8::OutputStream* stream) {
3093-
DisallowHeapAllocation no_heap_allocation;
30943093
v8::base::ElapsedTimer timer;
30953094
timer.Start();
30963095
DCHECK_NULL(writer_);
@@ -3450,17 +3449,10 @@ void HeapSnapshotJSONSerializer::SerializeTraceNodeInfos() {
34503449
// The cast is safe because script id is a non-negative Smi.
34513450
buffer_pos =
34523451
utoa(static_cast<unsigned>(info->script_id), buffer, buffer_pos);
3453-
3454-
auto& line_ends = snapshot_->GetScriptLineEnds(info->script_id);
3455-
int line = -1;
3456-
int column = -1;
3457-
Script::GetLineColumnWithLineEnds(info->start_position, line, column,
3458-
line_ends);
3459-
34603452
buffer[buffer_pos++] = ',';
3461-
buffer_pos = SerializePosition(line, buffer, buffer_pos);
3453+
buffer_pos = SerializePosition(info->line, buffer, buffer_pos);
34623454
buffer[buffer_pos++] = ',';
3463-
buffer_pos = SerializePosition(column, buffer, buffer_pos);
3455+
buffer_pos = SerializePosition(info->column, buffer, buffer_pos);
34643456
buffer[buffer_pos++] = '\n';
34653457
buffer[buffer_pos++] = '\0';
34663458
writer_->AddString(buffer.begin());

0 commit comments

Comments
 (0)