Skip to content

Make *-inl.h slim again #43712

Closed as not planned
Closed as not planned
Make *-inl.h slim again#43712
Labels
c++Issues and PRs that require attention from people who are familiar with C++.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Description

@bnoordhuis
bnoordhuis
opened on Jul 7, 2022

Inspired by me setting a breakpoint on a function and gdb telling me there are 12 locations...

The idea behind the *-inl.h files is to provide inline definitions of short functions. Large gobs of code don't belong in them because they negatively impact:

  1. Compile times
  2. Binary size (duplication)
  3. Run-time performance (instruction cache pressure)

Rules of thumb:

  1. Functions with few or infrequent callers should (judgment call) live in a .cc file
  2. Functions over 4-5 lines that aren't template functions should live in a .cc file
  3. Macros like CHECK(..) expand to multiple lines of code and should be counted as such

Good (short, many callers):

node/src/env-inl.h

Lines 56 to 58 in 7d13f5e

inline v8::Isolate* IsolateData::isolate() const {
return isolate_;
}

Not good (big, only two infrequent callers):

node/src/env-inl.h

Lines 349 to 365 in 7d13f5e

inline void Environment::AssignToContext(v8::Local<v8::Context> context,
const ContextInfo& info) {
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kEnvironment, this);
// Used by Environment::GetCurrent to know that we are on a node context.
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kContextTag, Environment::kNodeContextTagPtr);
// Used to retrieve bindings
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kBindingListIndex, &(this->bindings_));
#if HAVE_INSPECTOR
inspector_agent()->ContextCreated(context, info);
#endif // HAVE_INSPECTOR
this->async_hooks()->AddContext(context);
}

Questionable (single infrequent caller):

node/src/env-inl.h

Lines 510 to 518 in 7d13f5e

inline void Environment::TryLoadAddon(
const char* filename,
int flags,
const std::function<bool(binding::DLib*)>& was_loaded) {
loaded_addons_.emplace_back(filename, flags);
if (!was_loaded(&loaded_addons_.back())) {
loaded_addons_.pop_back();
}
}

The most commonly used *-inl.h files are:

$ find src -type f | xargs perl -ne 'print "$1\n" if /#\s*include "([^"]+-inl.h)"/' | sort | uniq -c | sort -r | head -9
  91 env-inl.h
  74 util-inl.h
  52 memory_tracker-inl.h
  42 base_object-inl.h
  35 async_wrap-inl.h
  29 debug_utils-inl.h
  19 threadpoolwork-inl.h
  18 node_process-inl.h
  15 stream_base-inl.h

Activity

  1. bnoordhuis
    added
    c++Issues and PRs that require attention from people who are familiar with C++.
    on Jul 7, 2022
  2. nodejs-github-bot
    added a commit that references this issue on Jul 13, 2022

    src: slim down env-inl.h

    3f568e0

  3. kvakil commented on Jul 22, 2022

    @kvakil
    kvakil
    on Jul 22, 2022
    Contributor

    I ran rtags and hacked together something to put the results into SQLite.
    The line count is approximate (it's literally based on the number of visual lines
    the function takes up), but looks to already identify some big ones:

    non-template inlined functions which are >= 8 lines 
    > select function_name, file || ':' || line || ':' || column as location, callers, number_of_lines from inlines where function_name NOT LIKE '%<%' and number_of_lines >= 8;
function_name                                       location                        callers     number_of_lines
--------------------------------------------------  ------------------------------  ----------  ---------------
unsigned long base64_encode                         src/base64-inl.h:124:15         5           65             
node::StreamWriteResult StreamBase::Write           src/stream_base-inl.h:163:31    7           55             
void NodeTraceStateObserver::OnTraceEnabled         src/node_v8_platform-inl.h:25:  2           42             
int StreamBase::Shutdown                            src/stream_base-inl.h:125:17    6           36             
void StreamResource::RemoveStreamListener           src/stream_base-inl.h:70:22     8           22             
void SwapBytes16                                    src/util-inl.h:216:6            6           21             
void SwapBytes32                                    src/util-inl.h:239:6            1           21             
void SwapBytes64                                    src/util-inl.h:262:6            1           21             
node::MemoryRetainerNode *MemoryTracker::AddNode    src/memory_tracker-inl.h:302:3  1           18             
node::fs::FSReqBase *GetReqWrap                     src/node_file-inl.h:233:12      37          18             
void V8Platform::Initialize                         src/node_v8_platform-inl.h:87:  1           18             
node::Environment *Environment::GetCurrent          src/env-inl.h:179:34            93          16             
void MemoryTracker::Track                           src/memory_tracker-inl.h:273:2  4           15             
void V8Platform::StartTracingAgent                  src/node_v8_platform-inl.h:126  2           15             
void ThreadPoolWork::ScheduleWork                   src/threadpoolwork-inl.h:32:22  4           15             
unsigned long Histogram::RecordDelta                src/histogram-inl.h:82:21       2           14             
void FSReqBase::Init                                src/node_file-inl.h:51:17       1           14             
void StreamReq::Done                                src/stream_base-inl.h:281:17    11          14             
void V8Platform::Dispose                            src/node_v8_platform-inl.h:107  2           13             
MemoryRetainerNode::MemoryRetainerNode              src/memory_tracker-inl.h:24:10  1           10             
void MemoryTracker::TrackField                      src/memory_tracker-inl.h:98:21  23          10             
node::MemoryRetainerNode *MemoryTracker::AddNode    src/memory_tracker-inl.h:322:3  3           9              
SlicedArguments::SlicedArguments                    src/util-inl.h:504:18           2           9              
bool Histogram::Record                              src/histogram-inl.h:72:17       1           8              
const char *GetNodeName                             src/memory_tracker-inl.h:12:20  3           8              
MemoryRetainerNode::MemoryRetainerNode              src/memory_tracker-inl.h:36:10  1           8              
void StreamResource::PushStreamListener             src/stream_base-inl.h:60:22     8           8              
bool StringEqualNoCaseN                             src/util-inl.h:315:6            13          8              
bool IsSafeJsInt                                    src/util-inl.h:549:13           11          8              
bool FastStringKey::operator==                      src/util-inl.h:573:31           1           8

    Also can look at number of callers. There are some mistakes here, f.ex. some
    are marked as having zero callers when they clearly have some.

    non-template inlined functions with <= 2 callsites 
    > select function_name, file || ':' || line || ':' || column as location, callers, number_of_lines from inlines where function_name NOT LIKE '%<%' and callers <= 2 order by number_of_lines desc limit 30;
function_name                                       location                        callers     number_of_lines
--------------------------------------------------  ------------------------------  ----------  ---------------
void NodeTraceStateObserver::OnTraceEnabled         src/node_v8_platform-inl.h:25:  2           42
void SwapBytes32                                    src/util-inl.h:239:6            1           21
void SwapBytes64                                    src/util-inl.h:262:6            1           21
node::MemoryRetainerNode *MemoryTracker::AddNode    src/memory_tracker-inl.h:302:3  1           18
void V8Platform::Initialize                         src/node_v8_platform-inl.h:87:  1           18
void V8Platform::StartTracingAgent                  src/node_v8_platform-inl.h:126  2           15
unsigned long Histogram::RecordDelta                src/histogram-inl.h:82:21       2           14
void FSReqBase::Init                                src/node_file-inl.h:51:17       1           14
void V8Platform::Dispose                            src/node_v8_platform-inl.h:107  2           13
MemoryRetainerNode::MemoryRetainerNode              src/memory_tracker-inl.h:24:10  1           10
SlicedArguments::SlicedArguments                    src/util-inl.h:504:18           2           9
bool Histogram::Record                              src/histogram-inl.h:72:17       1           8
MemoryRetainerNode::MemoryRetainerNode              src/memory_tracker-inl.h:36:10  1           8
bool FastStringKey::operator==                      src/util-inl.h:573:31           1           8
bool Environment::no_browser_globals                src/env-inl.h:675:26            2           7
double Histogram::Add                               src/histogram-inl.h:20:19       1           7
FSReqBase::FSReqBase                                src/node_file-inl.h:42:12       1           7
unsigned long FastStringKey::HashImpl               src/util-inl.h:559:33           1           7
bool BaseObject::IsWeakOrDetached                   src/base_object-inl.h:87:18     2           6
void Environment::DoneBootstrapping                 src/env-inl.h:621:26            2           6
void MemoryTracker::TrackInlineFieldWithSize        src/memory_tracker-inl.h:84:21  1           6
node::MemoryRetainerNode *MemoryTracker::PushNode   src/memory_tracker-inl.h:340:3  2           6
FSReqCallback::FSReqCallback                        src/node_file-inl.h:77:16       1           6
void StreamReq::AttachToObject                      src/stream_base-inl.h:20:17     1           6
void BaseObject::ClearWeak                          src/base_object-inl.h:80:18     2           5
DiagnosticFilename::DiagnosticFilename              src/diagnosticfilename-inl.h:1  1           5
void ShouldNotAbortOnUncaughtScope::Close           src/env-inl.h:406:37            2           5
node::BaseObject *CleanupHookCallback::GetBaseObje  src/env-inl.h:814:34            1           5
void MemoryTracker::TrackInlineField                src/memory_tracker-inl.h:290:2  0           5

    Hopefully this provides a burndown list for people looking to work on this.

    Here is a (gzipped) copy of the database for those looking for a
    finer-grained analysis: inl.db.gz

  4. danielleadams
    added a commit that references this issue on Jul 26, 2022

    src: slim down env-inl.h

    8432d65
  5. targos
    added a commit that references this issue on Jul 31, 2022

    src: slim down env-inl.h

    364b67c
  6. guangwong
    added a commit that references this issue on Oct 10, 2022

    src: slim down env-inl.h

    3515fae

  7. lilsweetcaligula commented on Mar 5, 2023

    @lilsweetcaligula
    lilsweetcaligula
    on Mar 5, 2023
    Contributor

    @bnoordhuis I have refactored stream_base-inl.h and managed to get the binary size down by 4 bytes (in 9 compilations out of 10). Would you accept a PR?

  8. bnoordhuis commented on Mar 6, 2023

    @bnoordhuis
    bnoordhuis
    on Mar 6, 2023
    MemberAuthor

    @lilsweetcaligula I'm going to guess most calls come from src/stream_base.cc if moving code around only slims down the binary by 4 bytes. But sure, pull request welcome.

  9. nodejs-github-bot
    added a commit that references this issue on Apr 11, 2023

    src: slim down stream_base-inl.h

    97d3912
  10. targos
    added a commit that references this issue on May 2, 2023

    src: slim down stream_base-inl.h

    adc1601
  11. danielleadams
    added a commit that references this issue on Jul 6, 2023

    src: slim down stream_base-inl.h

    c19bdae
  12. MoLow
    added a commit that references this issue on Jul 6, 2023

    src: slim down stream_base-inl.h

    e88e249

  13. github-actions commented on Jun 25, 2026

    @github-actions
    github-actionsbot
    on Jun 25, 2026 – with GitHub Actions
    Contributor

    This issue has been marked as stale due to 210 days of inactivity.
    It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

  14. github-actions
    added
    staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.
    on Jun 25, 2026

  15. github-actions commented on Jul 26, 2026

    @github-actions
    github-actionsbot
    on Jul 26, 2026 – with GitHub Actions
    Contributor

    This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
    If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.

  16. github-actions
    closed this as not plannedon Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    c++Issues and PRs that require attention from people who are familiar with C++.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions