5412666: [heap] Also avoid heap allocation for allocation tracked fun…

…ctions | https://chromium-review.googlesource.com/c/v8/v8/+/5412666

patches/v8/.patches

@@ -1,2 +1,3 @@
 chore_allow_customizing_microtask_policy_per_context.patch
 deps_add_v8_object_setinternalfieldfornodecore.patch
+revert_avoid_heap_allocation.patch

patches/v8/revert_avoid_heap_allocation.patch

@@ -0,0 +1,349 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Keeley Hammond <[email protected]>
+Date: Tue, 7 May 2024 14:45:12 -0700
+Subject: Revert "[heap] Also avoid heap allocation for allocation tracked
+ functions"
+
+This reverts commit 6e70dc9e73af2a930074f4f000864fbc65515b74. This CL was
+causing several Node failures in V8 tests. This patch can be removed when
+upstream is fixed.
+
+diff --git a/src/objects/objects.cc b/src/objects/objects.cc
+index 15e428ad2a870cae1c9efcb7fa2426b88d25b3c7..9c69155104d71cb1126da886e2ad0b15b66d6e30 100644
+--- a/src/objects/objects.cc
++++ b/src/objects/objects.cc
+@@ -4404,11 +4404,25 @@ int GetLength(const String::LineEndsVector& vector) {
+ }
+
+ int GetLength(const Tagged<FixedArray>& array) { return array->length(); }
++} // namespace
++
++void Script::AddPositionInfoOffset(PositionInfo* info,
++ OffsetFlag offset_flag) const {
++ // Add offsets if requested.
++ if (offset_flag == OffsetFlag::kWithOffset) {
++ if (info->line == 0) {
++ info->column += column_offset();
++ }
++ info->line += line_offset();
++ } else {
++ DCHECK_EQ(offset_flag, OffsetFlag::kNoOffset);
++ }
++}
+
+ template <typename LineEndsContainer>
+-bool GetLineEndsContainerPositionInfo(const LineEndsContainer& ends,
+- int position, Script::PositionInfo* info,
+- const DisallowGarbageCollection& no_gc) {
++bool Script::GetPositionInfoInternal(
++ const LineEndsContainer& ends, int position, Script::PositionInfo* info,
++ const DisallowGarbageCollection& no_gc) const {
+ const int ends_len = GetLength(ends);
+ if (ends_len == 0) return false;
+
+@@ -4447,31 +4461,6 @@ bool GetLineEndsContainerPositionInfo(const LineEndsContainer& ends,
+ info->column = position - info->line_start;
+ }
+
+- return true;
+-}
+-
+-} // namespace
+-
+-void Script::AddPositionInfoOffset(PositionInfo* info,
+- OffsetFlag offset_flag) const {
+- // Add offsets if requested.
+- if (offset_flag == OffsetFlag::kWithOffset) {
+- if (info->line == 0) {
+- info->column += column_offset();
+- }
+- info->line += line_offset();
+- } else {
+- DCHECK_EQ(offset_flag, OffsetFlag::kNoOffset);
+- }
+-}
+-
+-template <typename LineEndsContainer>
+-bool Script::GetPositionInfoInternal(
+- const LineEndsContainer& ends, int position, Script::PositionInfo* info,
+- const DisallowGarbageCollection& no_gc) const {
+- if (!GetLineEndsContainerPositionInfo(ends, position, info, no_gc))
+- return false;
+-
+ // Line end is position of the linebreak character.
+ info->line_end = GetLineEnd(ends, info->line);
+ if (info->line_end > 0) {
+@@ -4540,23 +4529,6 @@ bool Script::GetPositionInfoWithLineEnds(
+ return true;
+ }
+
+-bool Script::GetLineColumnWithLineEnds(
+- int position, int& line, int& column,
+- const String::LineEndsVector& line_ends) {
+- DisallowGarbageCollection no_gc;
+- PositionInfo info;
+- if (!GetLineEndsContainerPositionInfo(line_ends, position, &info, no_gc)) {
+- line = -1;
+- column = -1;
+- return false;
+- }
+-
+- line = info.line;
+- column = info.column;
+-
+- return true;
+-}
+-
+ int Script::GetColumnNumber(Handle<Script> script, int code_pos) {
+ PositionInfo info;
+ GetPositionInfo(script, code_pos, &info);
+diff --git a/src/objects/script.h b/src/objects/script.h
+index bfa0dc0e9b7625636c21620563ef0a19fcf20900..8b89639dd78a80cfae570a4746f391b1359d65a7 100644
+--- a/src/objects/script.h
++++ b/src/objects/script.h
+@@ -205,9 +205,6 @@ class Script : public TorqueGeneratedScript<Script, Struct> {
+ static bool GetPositionInfo(Handle<Script> script, int position,
+ PositionInfo* info,
+ OffsetFlag offset_flag = OffsetFlag::kWithOffset);
+- static bool GetLineColumnWithLineEnds(
+- int position, int& line, int& column,
+- const String::LineEndsVector& line_ends);
+ V8_EXPORT_PRIVATE bool GetPositionInfo(
+ int position, PositionInfo* info,
+ OffsetFlag offset_flag = OffsetFlag::kWithOffset) const;
+diff --git a/src/profiler/allocation-tracker.cc b/src/profiler/allocation-tracker.cc
+index 4f9e3bb8a5d2b20bba2cc09770872b6ab2a546e8..eac8c082aee0a122282f70956816a25c1ee30112 100644
+--- a/src/profiler/allocation-tracker.cc
++++ b/src/profiler/allocation-tracker.cc
+@@ -96,7 +96,10 @@ AllocationTracker::FunctionInfo::FunctionInfo()
+ function_id(0),
+ script_name(""),
+ script_id(0),
+- start_position(-1) {}
++ line(-1),
++ column(-1) {
++}
++
+
+ void AddressToTraceMap::AddRange(Address start, int size,
+ unsigned trace_node_id) {
+@@ -178,10 +181,23 @@ AllocationTracker::AllocationTracker(HeapObjectsMap* ids, StringsStorage* names)
+ function_info_list_.push_back(info);
+ }
+
++
+ AllocationTracker::~AllocationTracker() {
++ for (UnresolvedLocation* location : unresolved_locations_) delete location;
+ for (FunctionInfo* info : function_info_list_) delete info;
+ }
+
++
++void AllocationTracker::PrepareForSerialization() {
++ for (UnresolvedLocation* location : unresolved_locations_) {
++ location->Resolve();
++ delete location;
++ }
++ unresolved_locations_.clear();
++ unresolved_locations_.shrink_to_fit();
++}
++
++
+ void AllocationTracker::AllocationEvent(Address addr, int size) {
+ DisallowGarbageCollection no_gc;
+ Heap* heap = ids_->heap();
+@@ -235,7 +251,10 @@ unsigned AllocationTracker::AddFunctionInfo(Tagged<SharedFunctionInfo> shared,
+ info->script_name = names_->GetName(name);
+ }
+ info->script_id = script->id();
+- info->start_position = shared->StartPosition();
++ // Converting start offset into line and column may cause heap
++ // allocations so we postpone them until snapshot serialization.
++ unresolved_locations_.push_back(
++ new UnresolvedLocation(script, shared->StartPosition(), info));
+ }
+ entry->value = reinterpret_cast<void*>(function_info_list_.size());
+ function_info_list_.push_back(info);
+@@ -255,5 +274,39 @@ unsigned AllocationTracker::functionInfoIndexForVMState(StateTag state) {
+ return info_index_for_other_state_;
+ }
+
++AllocationTracker::UnresolvedLocation::UnresolvedLocation(Tagged<Script> script,
++ int start,
++ FunctionInfo* info)
++ : start_position_(start), info_(info) {
++ script_ = script->GetIsolate()->global_handles()->Create(script);
++ GlobalHandles::MakeWeak(script_.location(), this, &HandleWeakScript,
++ v8::WeakCallbackType::kParameter);
++}
++
++AllocationTracker::UnresolvedLocation::~UnresolvedLocation() {
++ if (!script_.is_null()) {
++ GlobalHandles::Destroy(script_.location());
++ }
++}
++
++
++void AllocationTracker::UnresolvedLocation::Resolve() {
++ if (script_.is_null()) return;
++ HandleScope scope(script_->GetIsolate());
++ Script::PositionInfo pos_info;
++ Script::GetPositionInfo(script_, start_position_, &pos_info);
++ info_->line = pos_info.line;
++ info_->column = pos_info.column;
++}
++
++void AllocationTracker::UnresolvedLocation::HandleWeakScript(
++ const v8::WeakCallbackInfo<void>& data) {
++ UnresolvedLocation* loc =
++ reinterpret_cast<UnresolvedLocation*>(data.GetParameter());
++ GlobalHandles::Destroy(loc->script_.location());
++ loc->script_ = Handle<Script>::null();
++}
++
++
+ } // namespace internal
+ } // namespace v8
+diff --git a/src/profiler/allocation-tracker.h b/src/profiler/allocation-tracker.h
+index d04bb9bf73b4b5725c99dade26510f4964ec0d4b..bb59c92a780160c4168349bfc5d523dffff5684f 100644
+--- a/src/profiler/allocation-tracker.h
++++ b/src/profiler/allocation-tracker.h
+@@ -104,7 +104,8 @@ class AllocationTracker {
+ SnapshotObjectId function_id;
+ const char* script_name;
+ int script_id;
+- int start_position;
++ int line;
++ int column;
+ };
+
+ AllocationTracker(HeapObjectsMap* ids, StringsStorage* names);
+@@ -112,6 +113,7 @@ class AllocationTracker {
+ AllocationTracker(const AllocationTracker&) = delete;
+ AllocationTracker& operator=(const AllocationTracker&) = delete;
+
++ V8_EXPORT_PRIVATE void PrepareForSerialization();
+ void AllocationEvent(Address addr, int size);
+
+ AllocationTraceTree* trace_tree() { return &trace_tree_; }
+@@ -125,6 +127,20 @@ class AllocationTracker {
+ SnapshotObjectId id);
+ unsigned functionInfoIndexForVMState(StateTag state);
+
++ class UnresolvedLocation {
++ public:
++ UnresolvedLocation(Tagged<Script> script, int start, FunctionInfo* info);
++ ~UnresolvedLocation();
++ void Resolve();
++
++ private:
++ static void HandleWeakScript(const v8::WeakCallbackInfo<void>& data);
++
++ Handle<Script> script_;
++ int start_position_;
++ FunctionInfo* info_;
++ };
++
+ static const int kMaxAllocationTraceLength = 64;
+ HeapObjectsMap* ids_;
+ StringsStorage* names_;
+@@ -132,6 +148,7 @@ class AllocationTracker {
+ unsigned allocation_trace_buffer_[kMaxAllocationTraceLength];
+ std::vector<FunctionInfo*> function_info_list_;
+ base::HashMap id_to_function_info_index_;
++ std::vector<UnresolvedLocation*> unresolved_locations_;
+ unsigned info_index_for_other_state_;
+ AddressToTraceMap address_to_trace_;
+ };
+diff --git a/src/profiler/heap-snapshot-generator.cc b/src/profiler/heap-snapshot-generator.cc
+index 4f3494d4677312ebc6b7e86ef64e08b434e9f77b..f449104937a46bc7d2f9ba69b41f33875ce692ac 100644
+--- a/src/profiler/heap-snapshot-generator.cc
++++ b/src/profiler/heap-snapshot-generator.cc
+@@ -3090,6 +3090,10 @@ void HeapSnapshotJSONSerializer::Serialize(v8::OutputStream* stream) {
+ DisallowHeapAllocation no_heap_allocation;
+ v8::base::ElapsedTimer timer;
+ timer.Start();
++ if (AllocationTracker* allocation_tracker =
++ snapshot_->profiler()->allocation_tracker()) {
++ allocation_tracker->PrepareForSerialization();
++ }
+ DCHECK_NULL(writer_);
+ writer_ = new OutputStreamWriter(stream);
+ SerializeImpl();
+@@ -3447,17 +3451,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());
+diff --git a/test/cctest/test-heap-profiler.cc b/test/cctest/test-heap-profiler.cc
+index 6e85e2c65d5542a2a67fa078d04ed5b82a89f0ff..a9528e3452b9873fb64da6aa9528d51239cb9afa 100644
+--- a/test/cctest/test-heap-profiler.cc
++++ b/test/cctest/test-heap-profiler.cc
+@@ -2841,6 +2841,8 @@ TEST(ArrayGrowLeftTrim) {
+ AllocationTracker* tracker =
+ reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
+ CHECK(tracker);
++ // Resolve all function locations.
++ tracker->PrepareForSerialization();
+ // Print for better diagnostics in case of failure.
+ tracker->trace_tree()->Print(tracker);
+
+@@ -2863,6 +2865,8 @@ TEST(TrackHeapAllocationsWithInlining) {
+ AllocationTracker* tracker =
+ reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
+ CHECK(tracker);
++ // Resolve all function locations.
++ tracker->PrepareForSerialization();
+ // Print for better diagnostics in case of failure.
+ tracker->trace_tree()->Print(tracker);
+
+@@ -2895,6 +2899,8 @@ TEST(TrackHeapAllocationsWithoutInlining) {
+ AllocationTracker* tracker =
+ reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
+ CHECK(tracker);
++ // Resolve all function locations.
++ tracker->PrepareForSerialization();
+ // Print for better diagnostics in case of failure.
+ tracker->trace_tree()->Print(tracker);
+
+@@ -2943,6 +2949,8 @@ TEST(TrackBumpPointerAllocations) {
+ AllocationTracker* tracker =
+ reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
+ CHECK(tracker);
++ // Resolve all function locations.
++ tracker->PrepareForSerialization();
+ // Print for better diagnostics in case of failure.
+ tracker->trace_tree()->Print(tracker);
+
+@@ -2967,6 +2975,8 @@ TEST(TrackBumpPointerAllocations) {
+ AllocationTracker* tracker =
+ reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
+ CHECK(tracker);
++ // Resolve all function locations.
++ tracker->PrepareForSerialization();
+ // Print for better diagnostics in case of failure.
+ tracker->trace_tree()->Print(tracker);
+
+@@ -2994,6 +3004,8 @@ TEST(TrackV8ApiAllocation) {
+ AllocationTracker* tracker =
+ reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
+ CHECK(tracker);
++ // Resolve all function locations.
++ tracker->PrepareForSerialization();
+ // Print for better diagnostics in case of failure.
+ tracker->trace_tree()->Print(tracker);
+