Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: use constant strings for memory info names #46087

Merged
merged 2 commits into from Jan 9, 2023
Merged

src: use constant strings for memory info names #46087

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a529c35
src: use constant strings for memory info names
legendecas Jan 4, 2023
b2eda31
fixup! src: use constant strings for memory info names
legendecas Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/async_wrap.cc
View file
Open in desktop
Expand Up @@ -673,13 +673,14 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
return ret;
}

std::string AsyncWrap::MemoryInfoName() const {
const char* AsyncWrap::MemoryInfoName() const {
return provider_names[provider_type()];
}

std::string AsyncWrap::diagnostic_name() const {
return MemoryInfoName() + " (" + std::to_string(env()->thread_id()) + ":" +
std::to_string(static_cast<int64_t>(async_id_)) + ")";
return std::string(MemoryInfoName()) + " (" +
std::to_string(env()->thread_id()) + ":" +
std::to_string(static_cast<int64_t>(async_id_)) + ")";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this would be a little more efficient (fewer allocations):

Suggested change
return std::string(MemoryInfoName()) + " (" +
std::to_string(env()->thread_id()) + ":" +
std::to_string(static_cast<int64_t>(async_id_)) + ")";
char buf[64];
snprintf(buf, sizeof(buf), "%s(%" PRIu64 ":%.0f)",
MemoryInfoName(), env()->thread_id(), async_id_);
return buf;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t worry about efficiency for debug-only code too much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, diagnostic_name() is only invoked on debug logs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with suggestion applied.

}

Local<Object> AsyncWrap::GetOwner() {
Expand Down
2 changes: 1 addition & 1 deletion src/async_wrap.h
View file
Open in desktop
Expand Up @@ -209,7 +209,7 @@ class AsyncWrap : public BaseObject {
v8::Local<v8::Value>* argv);

virtual std::string diagnostic_name() const;
std::string MemoryInfoName() const override;
const char* MemoryInfoName() const override;

static void WeakCallback(const v8::WeakCallbackInfo<DestroyParam> &info);

Expand Down
2 changes: 1 addition & 1 deletion src/crypto/crypto_util.h
View file
Open in desktop
Expand Up @@ -398,7 +398,7 @@ class CryptoJob : public AsyncWrap, public ThreadPoolWork {

AdditionalParams* params() { return &params_; }

std::string MemoryInfoName() const override {
const char* MemoryInfoName() const override {
return CryptoJobTraits::JobName;
}

Expand Down
4 changes: 2 additions & 2 deletions src/memory_tracker-inl.h
View file
Open in desktop
Expand Up @@ -44,7 +44,7 @@ class MemoryRetainerNode : public v8::EmbedderGraph::Node {
is_root_node_ = is_root_node;
}

const char* Name() override { return name_.c_str(); }
const char* Name() override { return name_; }
const char* NamePrefix() override { return "Node /"; }
size_t SizeInBytes() override { return size_; }
// TODO(addaleax): Merging this with the "official" WrapperNode() method
Expand Down Expand Up @@ -75,7 +75,7 @@ class MemoryRetainerNode : public v8::EmbedderGraph::Node {

// Otherwise (retainer == nullptr), we set these fields in an ad-hoc way
bool is_root_node_ = false;
std::string name_;
const char* name_;
size_t size_ = 0;
v8::EmbedderGraph::Node::Detachedness detachedness_ =
v8::EmbedderGraph::Node::Detachedness::kUnknown;
Expand Down
6 changes: 3 additions & 3 deletions src/memory_tracker.h
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ namespace node {

// Set the node name of a MemoryRetainer to klass
#define SET_MEMORY_INFO_NAME(Klass) \
inline std::string MemoryInfoName() const override { return #Klass; }
inline const char* MemoryInfoName() const override { return #Klass; }

// Set the self size of a MemoryRetainer to the stack-allocated size of a
// certain class
Expand Down Expand Up @@ -68,7 +68,7 @@ class CleanupHookCallback;
* }
*
* // Or use SET_MEMORY_INFO_NAME(ExampleRetainer)
* std::string MemoryInfoName() const override {
* const char* MemoryInfoName() const override {
* return "ExampleRetainer";
* }
*
Expand Down Expand Up @@ -119,7 +119,7 @@ class MemoryRetainer {
// where all the edges start from the node of the current retainer,
// and point to the nodes as specified by tracker->Track* calls.
virtual void MemoryInfo(MemoryTracker* tracker) const = 0;
virtual std::string MemoryInfoName() const = 0;
virtual const char* MemoryInfoName() const = 0;
virtual size_t SelfSize() const = 0;

virtual v8::Local<v8::Object> WrappedObject() const {
Expand Down
4 changes: 2 additions & 2 deletions src/node_process_methods.cc
View file
Open in desktop
Expand Up @@ -283,14 +283,14 @@ static void GetActiveResourcesInfo(const FunctionCallbackInfo<Value>& args) {
AsyncWrap* w = req_wrap->GetAsyncWrap();
if (w->persistent().IsEmpty()) continue;
resources_info.emplace_back(
OneByteString(env->isolate(), w->MemoryInfoName().c_str()));
OneByteString(env->isolate(), w->MemoryInfoName()));
}

// Active handles
for (HandleWrap* w : *env->handle_wrap_queue()) {
if (w->persistent().IsEmpty() || !HandleWrap::HasRef(w)) continue;
resources_info.emplace_back(
OneByteString(env->isolate(), w->MemoryInfoName().c_str()));
OneByteString(env->isolate(), w->MemoryInfoName()));
}

// Active timeouts
Expand Down
2 changes: 1 addition & 1 deletion src/node_realm.cc
View file
Open in desktop
Expand Up @@ -357,7 +357,7 @@ void Realm::VerifyNoStrongBaseObjects() {
if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return;
fprintf(stderr,
"Found bad BaseObject during clean exit: %s\n",
obj->MemoryInfoName().c_str());
obj->MemoryInfoName());
fflush(stderr);
ABORT();
});
Expand Down
2 changes: 1 addition & 1 deletion src/tcp_wrap.h
View file
Open in desktop
Expand Up @@ -50,7 +50,7 @@ class TCPWrap : public ConnectionWrap<TCPWrap, uv_tcp_t> {

SET_NO_MEMORY_INFO()
SET_SELF_SIZE(TCPWrap)
std::string MemoryInfoName() const override {
const char* MemoryInfoName() const override {
switch (provider_type()) {
case ProviderType::PROVIDER_TCPWRAP:
return "TCPSocketWrap";
Expand Down