-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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.
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
src: use constant strings for memory info names #46087
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think. It's hard to tell whether all names are indeed static strings and no real way to enforce that at compile time...
src/async_wrap.cc
Outdated
return std::string(MemoryInfoName()) + " (" + | ||
std::to_string(env()->thread_id()) + ":" + | ||
std::to_string(static_cast<int64_t>(async_id_)) + ")"; |
There was a problem hiding this comment.
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):
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with suggestion applied.
Landed in 80fa25b |
PR-URL: nodejs#46087 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #46087 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #46087 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #46087 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
v8::EmbedderGraph::Node::Name()
expects constant strings. There is no need fornode::MemoryRetainer::MemoryInfoName()
to return a copy of the name string when building an embedder graph for heap snapshots.