Closed Bug 1661242 Opened 1 year ago Closed 1 year ago

Declare nsCOMPtr_base::begin_assignment MOZ_ALWAYS_INLINE

Categories

(Core :: XPCOM, task)

task

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: sg, Assigned: sg)

Details

Attachments

(1 file)

In the typical use case, nsCOMPtr_base::begin_assignment is actually a no-op, e.g. when doing:

nsCOMPtr<nsIFile> file;
MOZ_TRY(anotherFile->Clone(getter_AddRefs(file));

Yet, at least on my Linux build, the compiler/linker (with LTO) chooses not to inline those calls in many situations, which bloats the code and is unnecessarily inefficient, and will affect dependent inlining decisions etc.

While there are certainly some situations where it can't be optimized away entirely, by declaring nsCOMPtr_base::begin_assignment MOZ_ALWAYS_INLINE (and moving its definition to the header), I actually get a smaller overall binary (in terms of VM_SIZE/.text section):

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1%  +350Ki  [ = ]       0    .debug_loc
  +0.0%  +168Ki  [ = ]       0    .debug_info
  +0.1% +57.6Ki  [ = ]       0    .debug_ranges
   +26% +1.69Ki  [ = ]       0    [Unmapped]
  +0.0%    +766  [ = ]       0    .debug_abbrev
  -0.0%    -384  -0.0%    -384    .eh_frame_hdr
  -0.0% -1.12Ki  [ = ]       0    .symtab
  -0.0% -2.16Ki  -0.0% -2.16Ki    .eh_frame
  -0.0% -2.99Ki  [ = ]       0    .debug_str
  -0.0% -3.35Ki  [ = ]       0    .strtab
  -0.0% -13.7Ki  [ = ]       0    .debug_line
  -0.0% -19.2Ki  -0.0% -19.2Ki    .text
  +0.0%  +536Ki  -0.0% -21.7Ki    TOTAL

I don't know why it's like this; blame on this function goes back to hg@1. I didn't try tracing it through CVS history.

My suspicion is that with all the extra logging that nsCOMPtr can do, moving this function out of line significantly reduced code size/compilation time on debug builds back in the day. Except that assign_assuming_AddRef, which is where all the logging bloat would come from, is already declared inline in the class definition and reasonably widely used throughout nsCOMPtr. (Maybe assign_assuming_AddRef was not used quite so widely back in the day?) So it seems like moving begin_assignment to the header probably (?) wouldn't make anything worse?

(In reply to Nathan Froyd [:froydnj] from comment #1)

I don't know why it's like this; blame on this function goes back to hg@1. I didn't try tracing it through CVS history.

My suspicion is that with all the extra logging that nsCOMPtr can do, moving this function out of line significantly reduced code size/compilation time on debug builds back in the day. Except that assign_assuming_AddRef, which is where all the logging bloat would come from, is already declared inline in the class definition and reasonably widely used throughout nsCOMPtr. (Maybe assign_assuming_AddRef was not used quite so widely back in the day?) So it seems like moving begin_assignment to the header probably (?) wouldn't make anything worse?

To be sure, we could make it MOZ_NEVER_INLINE in debug builds (resp. when the logging is enabled) and MOZ_ALWAYS_INLINE in release builds?

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

(In reply to Nathan Froyd [:froydnj] from comment #1)

I don't know why it's like this; blame on this function goes back to hg@1. I didn't try tracing it through CVS history.

My suspicion is that with all the extra logging that nsCOMPtr can do, moving this function out of line significantly reduced code size/compilation time on debug builds back in the day. Except that assign_assuming_AddRef, which is where all the logging bloat would come from, is already declared inline in the class definition and reasonably widely used throughout nsCOMPtr. (Maybe assign_assuming_AddRef was not used quite so widely back in the day?) So it seems like moving begin_assignment to the header probably (?) wouldn't make anything worse?

To be sure, we could make it MOZ_NEVER_INLINE in debug builds (resp. when the logging is enabled) and MOZ_ALWAYS_INLINE in release builds?

This solution works for me.

Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9380c5da94ac
Declare nsCOMPtr_base::begin_assignment MOZ_ALWAYS_INLINE in release builds. r=froydnj
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.