Make *-inl.h slim again #43712

bnoordhuis · 2022-07-07T08:22:44Z

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:

Compile times
Binary size (duplication)
Run-time performance (instruction cache pressure)

Rules of thumb:

Functions with few or infrequent callers should (judgment call) live in a .cc file
Functions over 4-5 lines that aren't template functions should live in a .cc file
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

The text was updated successfully, but these errors were encountered:

Move big and/or infrequently used functions from env-inl.h to env.cc to speed up build times and reduce binary bloat. This commit also touches async_wrap-inl.h and base_object-inl.h because those are closely interwined with env-inl.h. Non-functional change. Refs: nodejs#43712

Move big and/or infrequently used functions from env-inl.h to env.cc to speed up build times and reduce binary bloat. This commit also touches async_wrap-inl.h and base_object-inl.h because those are closely interwined with env-inl.h. Non-functional change. Refs: #43712 PR-URL: #43745 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Richard Lau <[email protected]>

kvakil · 2022-07-22T06:15:24Z

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

Move big and/or infrequently used functions from env-inl.h to env.cc to speed up build times and reduce binary bloat. This commit also touches async_wrap-inl.h and base_object-inl.h because those are closely interwined with env-inl.h. Non-functional change. Refs: #43712 PR-URL: #43745 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Move big and/or infrequently used functions from env-inl.h to env.cc to speed up build times and reduce binary bloat. This commit also touches async_wrap-inl.h and base_object-inl.h because those are closely interwined with env-inl.h. Non-functional change. Refs: nodejs/node#43712 PR-URL: nodejs/node#43745 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Richard Lau <[email protected]>

lilsweetcaligula · 2023-03-05T21:56:53Z

@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?

bnoordhuis · 2023-03-06T11:33:31Z

@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.

PR-URL: #46972 Refs: #43712 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

PR-URL: nodejs#46972 Refs: nodejs#43712 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>

bnoordhuis added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 7, 2022

bnoordhuis mentioned this issue Jul 9, 2022

src: slim down env-inl.h #43745

Merged

lilsweetcaligula mentioned this issue Mar 3, 2023

src: slim down node_v8_platform-inl.h #46926

Closed

lilsweetcaligula mentioned this issue Mar 6, 2023

src: slim down stream_base-inl.h #46972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make *-inl.h slim again #43712

Make *-inl.h slim again #43712

bnoordhuis commented Jul 7, 2022 •

edited by VoltrexKeyva

Loading

kvakil commented Jul 22, 2022 •

edited

Loading

lilsweetcaligula commented Mar 5, 2023

bnoordhuis commented Mar 6, 2023

Make *-inl.h slim again #43712

Make *-inl.h slim again #43712

Comments

bnoordhuis commented Jul 7, 2022 • edited by VoltrexKeyva Loading

kvakil commented Jul 22, 2022 • edited Loading

lilsweetcaligula commented Mar 5, 2023

bnoordhuis commented Mar 6, 2023

bnoordhuis commented Jul 7, 2022 •

edited by VoltrexKeyva

Loading

kvakil commented Jul 22, 2022 •

edited

Loading