1661242 - Declare nsCOMPtr_base::begin_assignment MOZ_ALWAYS_INLINE

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Simon Giesecke [:sg] [he/him]]

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

Comment 1

[Nathan Froyd [:froydnj]]

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?

Comment 2

[Simon Giesecke [:sg] [he/him]]

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

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1661242 - Declare nsCOMPtr_base::begin_assignment MOZ_ALWAYS_INLINE in release builds. r=#xpcom-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9380c5da94ac
Declare nsCOMPtr_base::begin_assignment MOZ_ALWAYS_INLINE in release builds. r=froydnj

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9380c5da94ac