Skip to content

Commit

Permalink
src: fix AliasedBuffer memory attribution in heap snapshots
Browse files Browse the repository at this point in the history
Before the AliasedBuffers were represented solely by the TypedArrays
event though they also retain additional memory themselves.
Make the accounting more accurate by implementing MemoryRetainer in
AliasedBuffer.

PR-URL: nodejs/node#46817
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
sercher committed Apr 24, 2024
1 parent 6ee24bd commit 755be77
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 15 deletions.
19 changes: 19 additions & 0 deletions graal-nodejs/src/aliased_buffer-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,25 @@ void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
count_ = new_capacity;
}

template <typename NativeT, typename V8T>
inline size_t AliasedBufferBase<NativeT, V8T>::SelfSize() const {
return sizeof(*this);
}

#define VM(NativeT, V8T) \
template <> \
inline const char* AliasedBufferBase<NativeT, v8::V8T>::MemoryInfoName() \
const { \
return "Aliased" #V8T; \
} \
template <> \
inline void AliasedBufferBase<NativeT, v8::V8T>::MemoryInfo( \
node::MemoryTracker* tracker) const { \
tracker->TrackField("js_array", js_array_); \
}
ALIASED_BUFFER_LIST(VM)
#undef VM

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
8 changes: 7 additions & 1 deletion graal-nodejs/src/aliased_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <cinttypes>
#include "memory_tracker.h"
#include "v8.h"

namespace node {
Expand All @@ -28,7 +29,7 @@ typedef size_t AliasedBufferIndex;
* observed. Any notification APIs will be left as a future exercise.
*/
template <class NativeT, class V8T>
class AliasedBufferBase {
class AliasedBufferBase : public MemoryRetainer {
public:
static_assert(std::is_scalar<NativeT>::value);

Expand Down Expand Up @@ -157,6 +158,11 @@ class AliasedBufferBase {
// an owning `AliasedBufferBase`.
void reserve(size_t new_capacity);

inline size_t SelfSize() const override;

inline const char* MemoryInfoName() const override;
inline void MemoryInfo(node::MemoryTracker* tracker) const override;

private:
v8::Isolate* isolate_ = nullptr;
size_t count_ = 0;
Expand Down
9 changes: 1 addition & 8 deletions graal-nodejs/src/memory_tracker-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "aliased_buffer-inl.h"
#include "memory_tracker.h"
#include "util-inl.h"

namespace node {

Expand Down Expand Up @@ -270,13 +270,6 @@ void MemoryTracker::TrackInlineField(const char* name,
TrackInlineFieldWithSize(name, sizeof(value), "uv_async_t");
}

template <class NativeT, class V8T>
void MemoryTracker::TrackField(const char* name,
const AliasedBufferBase<NativeT, V8T>& value,
const char* node_name) {
TrackField(name, value.GetJSArray(), "AliasedBuffer");
}

void MemoryTracker::Track(const MemoryRetainer* retainer,
const char* edge_name) {
v8::HandleScope handle_scope(isolate_);
Expand Down
5 changes: 0 additions & 5 deletions graal-nodejs/src/memory_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "aliased_buffer.h"
#include "v8-profiler.h"

#include <uv.h>
Expand Down Expand Up @@ -238,10 +237,6 @@ class MemoryTracker {
inline void TrackInlineField(const char* edge_name,
const uv_async_t& value,
const char* node_name = nullptr);
template <class NativeT, class V8T>
inline void TrackField(const char* edge_name,
const AliasedBufferBase<NativeT, V8T>& value,
const char* node_name = nullptr);

// Put a memory container into the graph, create an edge from
// the current node if there is one on the stack.
Expand Down
2 changes: 1 addition & 1 deletion graal-nodejs/test/pummel/test-heapdump-fs-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ validateSnapshotNodes('Node / FSReqPromise', [
{
children: [
{ node_name: 'FSReqPromise', edge_name: 'native_to_javascript' },
{ node_name: 'Float64Array', edge_name: 'stats_field_array' },
{ node_name: 'Node / AliasedFloat64Array', edge_name: 'stats_field_array' },
],
},
]);

0 comments on commit 755be77

Please sign in to comment.