[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 <jdapena@igalia.com> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Simon Zünd <szuend@chromium.org> Cr-Commit-Position: refs/heads/main@{#94418}

Author: jdapena

src/profiler/allocation-tracker.cc

@@ -4,10 +4,13 @@
 
 #include "src/profiler/allocation-tracker.h"
 
+#include "src/api/api-inl.h"
+#include "src/api/api.h"
 #include "src/execution/frames-inl.h"
 #include "src/handles/global-handles-inl.h"
 #include "src/objects/objects-inl.h"
 #include "src/profiler/heap-snapshot-generator-inl.h"
+#include "src/utils/utils.h"
 
 namespace v8 {
 namespace internal {
@@ -96,7 +99,9 @@ AllocationTracker::FunctionInfo::FunctionInfo()
  function_id(0),
  script_name(""),
  script_id(0),
- start_position(-1) {}
+ start_position(-1),
+ line(-1),
+ column(-1) {}
 
 void AddressToTraceMap::AddRange(Address start, int size,
  unsigned trace_node_id) {
@@ -199,7 +204,7 @@ void AllocationTracker::AllocationEvent(Address addr, int size) {
  SnapshotObjectId id =
  ids_->FindOrAddEntry(shared.address(), shared->Size(),
  HeapObjectsMap::MarkEntryAccessed::kNo);
- allocation_trace_buffer_[length++] = AddFunctionInfo(shared, id);
+ allocation_trace_buffer_[length++] = AddFunctionInfo(shared, id, isolate);
  it.Advance();
  }
  if (length == 0) {
@@ -220,8 +225,60 @@ static uint32_t SnapshotObjectIdHash(SnapshotObjectId id) {
  return ComputeUnseededHash(static_cast<uint32_t>(id));
 }
 
+AllocationTracker::ScriptData::ScriptData(Tagged<Script> script,
+ Isolate* isolate,
+ AllocationTracker* tracker)
+ : script_id_(script->id()),
+ line_ends_(Script::GetLineEnds(isolate, handle(script, isolate))),
+ tracker_(tracker) {
+ DirectHandle<Script> script_direct_handle(script, isolate);
+ auto local_script = ToApiHandle<debug::Script>(script_direct_handle, isolate);
+ script_.Reset(local_script->GetIsolate(), local_script);
+ script_.SetWeak(this, &HandleWeakScript, v8::WeakCallbackType::kParameter);
+}
+
+AllocationTracker::ScriptData::~ScriptData() {
+ if (!script_.IsEmpty()) {
+ script_.ClearWeak();
+ }
+}
+
+void AllocationTracker::ScriptData::HandleWeakScript(
+ const v8::WeakCallbackInfo<ScriptData>& data) {
+ ScriptData* script_data = reinterpret_cast<ScriptData*>(data.GetParameter());
+ script_data->script_.ClearWeak();
+ script_data->script_.Reset();
+ script_data->tracker_->scripts_data_map_.erase(script_data->script_id_);
+}
+
+String::LineEndsVector& AllocationTracker::GetOrCreateLineEnds(
+ Tagged<Script> script, Isolate* isolate) {
+ auto it = scripts_data_map_.find(script->id());
+ if (it == scripts_data_map_.end()) {
+ auto inserted =
+ scripts_data_map_.try_emplace(script->id(), script, isolate, this);
+ CHECK(inserted.second);
+ return inserted.first->second.line_ends();
+ } else {
+ return it->second.line_ends();
+ }
+}
+
+Script::PositionInfo AllocationTracker::GetScriptPositionInfo(
+ Tagged<Script> script, Isolate* isolate, int start) {
+ Script::PositionInfo position_info;
+ if (script->has_line_ends()) {
+ script->GetPositionInfo(start, &position_info);
+ } else {
+ script->GetPositionInfoWithLineEnds(start, &position_info,
+ GetOrCreateLineEnds(script, isolate));
+ }
+ return position_info;
+}
+
 unsigned AllocationTracker::AddFunctionInfo(Tagged<SharedFunctionInfo> shared,
- SnapshotObjectId id) {
+ SnapshotObjectId id,
+ Isolate* isolate) {
  base::HashMap::Entry* entry = id_to_function_info_index_.LookupOrInsert(
  reinterpret_cast<void*>(id), SnapshotObjectIdHash(id));
  if (entry->value == nullptr) {
@@ -236,6 +293,10 @@ unsigned AllocationTracker::AddFunctionInfo(Tagged<SharedFunctionInfo> shared,
  }
  info->script_id = script->id();
  info->start_position = shared->StartPosition();
+ Script::PositionInfo position_info =
+ GetScriptPositionInfo(script, isolate, info->start_position);
+ info->line = position_info.line;
+ info->column = position_info.column;
  }
  entry->value = reinterpret_cast<void*>(function_info_list_.size());
  function_info_list_.push_back(info);

src/profiler/allocation-tracker.h

@@ -6,14 +6,18 @@
 #define V8_PROFILER_ALLOCATION_TRACKER_H_
 
 #include <map>
+#include <unordered_map>
 #include <vector>
 
 #include "include/v8-persistent-handle.h"
 #include "include/v8-profiler.h"
 #include "include/v8-unwinder.h"
 #include "src/base/hashmap.h"
 #include "src/base/vector.h"
+#include "src/debug/debug-interface.h"
 #include "src/handles/handles.h"
+#include "src/objects/script.h"
+#include "src/objects/string.h"
 
 namespace v8 {
 namespace internal {
@@ -105,6 +109,8 @@ class AllocationTracker {
  const char* script_name;
  int script_id;
  int start_position;
+ int line;
+ int column;
  };
 
  AllocationTracker(HeapObjectsMap* ids, StringsStorage* names);
@@ -121,8 +127,12 @@ class AllocationTracker {
  AddressToTraceMap* address_to_trace() { return &address_to_trace_; }
 
  private:
- unsigned AddFunctionInfo(Tagged<SharedFunctionInfo> info,
- SnapshotObjectId id);
+ unsigned AddFunctionInfo(Tagged<SharedFunctionInfo> info, SnapshotObjectId id,
+ Isolate* isolate);
+ String::LineEndsVector& GetOrCreateLineEnds(Tagged<Script> script,
+ Isolate* isolate);
+ Script::PositionInfo GetScriptPositionInfo(Tagged<Script> script,
+ Isolate* isolate, int start);
  unsigned functionInfoIndexForVMState(StateTag state);
 
  static const int kMaxAllocationTraceLength = 64;
@@ -134,6 +144,22 @@ class AllocationTracker {
  base::HashMap id_to_function_info_index_;
  unsigned info_index_for_other_state_;
  AddressToTraceMap address_to_trace_;
+ using ScriptId = int;
+ class ScriptData {
+ public:
+ ScriptData(Tagged<Script>, Isolate*, AllocationTracker*);
+ ~ScriptData();
+ String::LineEndsVector& line_ends() { return line_ends_; }
+
+ private:
+ static void HandleWeakScript(const v8::WeakCallbackInfo<ScriptData>&);
+ Global<debug::Script> script_;
+ ScriptId script_id_;
+ String::LineEndsVector line_ends_;
+ AllocationTracker* tracker_;
+ };
+ using ScriptsDataMap = std::unordered_map<ScriptId, ScriptData>;
+ ScriptsDataMap scripts_data_map_;
 };
 
 } // namespace internal

src/profiler/heap-snapshot-generator.cc

@@ -3090,7 +3090,6 @@ bool HeapSnapshotGenerator::FillReferences() {
 const int HeapSnapshotJSONSerializer::kNodeFieldsCount = 7;
 
 void HeapSnapshotJSONSerializer::Serialize(v8::OutputStream* stream) {
- DisallowHeapAllocation no_heap_allocation;
  v8::base::ElapsedTimer timer;
  timer.Start();
  DCHECK_NULL(writer_);
@@ -3450,17 +3449,10 @@ void HeapSnapshotJSONSerializer::SerializeTraceNodeInfos() {
  // The cast is safe because script id is a non-negative Smi.
  buffer_pos =
  utoa(static_cast<unsigned>(info->script_id), buffer, buffer_pos);
-
- auto& line_ends = snapshot_->GetScriptLineEnds(info->script_id);
- int line = -1;
- int column = -1;
- Script::GetLineColumnWithLineEnds(info->start_position, line, column,
- line_ends);
-
  buffer[buffer_pos++] = ',';
- buffer_pos = SerializePosition(line, buffer, buffer_pos);
+ buffer_pos = SerializePosition(info->line, buffer, buffer_pos);
  buffer[buffer_pos++] = ',';
- buffer_pos = SerializePosition(column, buffer, buffer_pos);
+ buffer_pos = SerializePosition(info->column, buffer, buffer_pos);
  buffer[buffer_pos++] = '\n';
  buffer[buffer_pos++] = '\0';
  writer_->AddString(buffer.begin());