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.

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: slim down node_v8_platform-inl.h #46926

Closed

Conversation

lilsweetcaligula
Copy link
Contributor

@lilsweetcaligula lilsweetcaligula commented Mar 3, 2023

De-inline and move big functions from node_v8_platform-inl.h to help reduce the binary size.

Non-functional change.

Refs: #43712

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Mar 3, 2023
@lilsweetcaligula lilsweetcaligula force-pushed the bug43712-node_v8_platform branch from 7e5ddd3 to 7ef80b8 Compare March 3, 2023 07:52
bool initialized_ = false;

#if NODE_USE_V8_PLATFORM
void Initialize(int thread_pool_size) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method definitions like this - wherein a method is non-inline if NODE_USE_V8_PLATFORM but inline otherwise - have been intentionally kept in the .h file, as opposed to moving them to a .cc file.

The idea being - to prevent confusion and mistakes due to code duplication of the NODE_USE_V8_PLATFORM if-else construct, e.g.:

// src/node_v8_platform.h

class NodeTraceStateObserver {

#if NODE_USE_V8_PLATFORM
  void Initialize(int thread_pool_size);
#else
  inline void Initialize(int thread_pool_size);
#endif // NODE_USE_V8_PLATFORM

}
// src/node_v8_platform.cc

#if NODE_USE_V8_PLATFORM
void NodeTraceStateObserver::Initialize(int thread_pool_size) {
  // ...
}
#else
inline void NodeTraceStateObserver::Initialize(int thread_pool_size) {
  // ...
}
#endif // NODE_USE_V8_PLATFORM

@lilsweetcaligula lilsweetcaligula marked this pull request as ready for review March 3, 2023 08:12
@lilsweetcaligula lilsweetcaligula force-pushed the bug43712-node_v8_platform branch from 7ef80b8 to 56af746 Compare March 4, 2023 02:46
@lilsweetcaligula
Copy link
Contributor Author

Ran the cpp linter. My bad for force-pushing.

@bnoordhuis
Copy link
Member

Moving code from foo-inl.h file to foo.h doesn't fix the code bloat problem. In fact, it probably makes it worse because foo.h is normally included more often than foo-inl.h.

The goal of #43712 is to move method definitions that are too big/infrequently called/etc. from foo.h or foo-inl.h to foo.cc.

@lilsweetcaligula
Copy link
Contributor Author

@bnoordhuis
Noted, thank you for taking a look. I have proceeded to extract the definitions of the de-inlined functions into a separate .cc file (pushed the commit for reference). However upon comparing the binary size (i.e. du ./out/Release/node) against the main branch, I have realized that the size did not change. As such, is it correct to assume that it makes no sense to proceed with further refactoring of node_v8_platform-inl.h? (And you are correct, - turns out, before the extraction the size was even 4 bytes bigger, yikes...)

Generally speaking, should binary size be the "go-to" litmus test when evaluating usefulness of this kind of refactor? My C++ is a bit rusty but I intend to continue working on #43712 to completion.

@bnoordhuis
Copy link
Member

Yes, binary size is the decisive criterion. If moving code from a .h to a .cc makes no meaningful difference, then it likely means that code is not called from many places; there are probably just one or two call sites.

I'm 85% sure that applies to node_v8_platform-inl.h. Most includes of that file could probably be switched to node_v8_platform.h to no ill effect.

@lilsweetcaligula
Copy link
Contributor Author

@bnoordhuis Noted. Thank you. Closing the PR.

@lilsweetcaligula lilsweetcaligula deleted the bug43712-node_v8_platform branch March 5, 2023 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants