768865 - Make NS_SUCCEEDED/NS_FAILED always inline functions for C++

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Currently we have

  #if defined(NS_STATIC_CHECKING) && defined(__cplusplus)
  inline int NS_FAILED(nsresult _nsresult) {
    return   _nsresult & 0x80000000;
  }

  inline int NS_SUCCEEDED(nsresult _nsresult) {
    return !(_nsresult & 0x80000000);
  }
  #else
  #define NS_FAILED(_nsresult)    (NS_UNLIKELY((_nsresult) & 0x80000000))
  #define NS_SUCCEEDED(_nsresult) (NS_LIKELY(!((_nsresult) & 0x80000000)))
  #endif

This was courtesy of bug 420933.  I'd like to remove the "defined(NS_STATIC_CHECKING) &&" part, so this is done for all C++ code.  Otherwise, macro expansions can be hard to read, like this assert from bug 767169:

  Assertion failure: (((__builtin_expect(!!(!((nsRange::CompareNodeToRange(lastCandidate, mRange, &nodeBefore, &nodeAfter)) & 0x80000000)), 1)))), at content/base/src/nsContentIterator.cpp:1295

This should read:

  Assertion failure: (NS_SUCCEEDED(nsRange::CompareNodeToRange(lastCandidate, mRange, &nodeBefore, &nodeAfter)), at content/base/src/nsContentIterator.cpp:1295

Should I keep the #if for __cplusplus, or can C handle inline functions on all the compilers we care about?

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1, just removes NS_STATIC_CHECKING check

So far only compile-tested, on localhost.  Try: https://tbpl.mozilla.org/?tree=Try&rev=fec17f9c1641

If you want, I'm happy to remove the check for __cplusplus and make these inline functions unconditionally, and for that matter convert every #define in the file to a const variable or inline function, to the extent that works.  I just don't know if that works or is desirable.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Talos try run as requested: https://tbpl.mozilla.org/?tree=Try&rev=3140d5509ecd

Comment 3

[Benjamin Smedberg]

Comment on attachment 637123 [details] [diff] [review]
Patch v1, just removes NS_STATIC_CHECKING check

r=me if this doesn't significantly affect codesize, performance, or windows linker memory usage in a negative way.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> r=me if this doesn't significantly affect codesize, performance, or windows
> linker memory usage in a negative way.

No one gave me clear steps on how to verify this when I asked on IRC, so I'm going to have to drop this bug.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Okay, so I'd really like to get this done as part of the nsresult cleanup -- not for tidiness, but for type safety.  If we don't make this an inline function, it would have to become something like

  #define NS_FAILED(_nsresult)    (NS_UNLIKELY(PRUint32(_nsresult) & 0x80000000))

which has zero type-safety, because of the cast.  Ehsan, do you have any thoughts on what kind of performance analysis is necessary to land this change?

Comment 6

[Benjamin Smedberg]

I think the easiest way to check whether this affects perf is to check it in on mozilla-central and immediately trigger a PGO build. Then watch the Talos numbers carefully, and also check the linker memory metric for the windows PGO build.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #6)
> I think the easiest way to check whether this affects perf is to check it in
> on mozilla-central and immediately trigger a PGO build. Then watch the Talos
> numbers carefully, and also check the linker memory metric for the windows
> PGO build.

I'm fine with that, and am perfectly willing to do it if someone tells me how, but I currently have no idea what any of it means.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from
> comment #6)
> > I think the easiest way to check whether this affects perf is to check it in
> > on mozilla-central and immediately trigger a PGO build. Then watch the Talos
> > numbers carefully, and also check the linker memory metric for the windows
> > PGO build.
> 
> I'm fine with that, and am perfectly willing to do it if someone tells me how,
> but I currently have no idea what any of it means.

Here's a short description of what Benjamin was talking about.  You land this to mozilla-central, then load up TBPL and find your push and click on "Self-serve Build API" at the top right section of the row corresponding to your build, then copy the changeset ID and enter it in the "create new PGO build" box at the end of that page.  PGO (profile-guided optimization) builds are special builds where we build Firefox once with instrumentation of function calls, then we launch it and perform some performance intensive stuff to let those instrumentations gather data, then we close it up and rebuild Firefox so that the compiler can use the information about how the generated code was run to decide on what optimizations to perform.  These builds tend to have better performance and are what we use for release builds, and they are slower to build (on Windows the build can take around 4 hours).

Once you have PGO builds on your push, you can use a tool like http://perf.snarkfest.net/compare-talos/ to compare the performance numbers to the previous revisions to make sure that it has not regressed.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/f7cf60910637

Comment 10

[(no longer active)]

There are no noticeable changes in the linker vmem size or codesighs.

Comment 11

[(no longer active)]

So, compare-talos seems to suggest regressions in WinXP dromaeo_css and dromaeo_dom <http://bit.ly/RkTZMV>, but I don't know where those are coming from.  For example, this is a chart of WinXP PGO dromaeo_css <http://graphs.mozilla.org/graph.html#tests=[[72,1,1]]&sel=1345131516356.1152,1345150065055&displayrange=7&datatype=running> and there are no values near 3500 there, and there doesn't seem to be any regressions.

So far there are no Talos regression emails for this either.