Last Comment Bug 768865 - Make NS_SUCCEEDED/NS_FAILED always inline functions for C++
: Make NS_SUCCEEDED/NS_FAILED always inline functions for C++
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 802473
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-27 06:37 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-10-16 20:54 PDT (History)
2 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1, just removes NS_STATIC_CHECKING check (764 bytes, patch)
2012-06-27 07:58 PDT, :Aryeh Gregor (away until August 15)
benjamin: review+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-06-27 06:37:07 PDT
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 (away until August 15) 2012-06-27 07:58:58 PDT
Created attachment 637123 [details] [diff] [review]
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 (away until August 15) 2012-07-02 07:57:42 PDT
Talos try run as requested: https://tbpl.mozilla.org/?tree=Try&rev=3140d5509ecd
Comment 3 Benjamin Smedberg [:bsmedberg] 2012-07-10 07:15:45 PDT
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 (away until August 15) 2012-07-10 08:18:15 PDT
(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 (away until August 15) 2012-08-07 01:36:52 PDT
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 [:bsmedberg] 2012-08-07 07:00:28 PDT
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 (away until August 15) 2012-08-13 05:18:02 PDT
(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 :Ehsan Akhgari (out sick) 2012-08-14 13:45:42 PDT
(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 (away until August 15) 2012-08-16 06:20:00 PDT
https://hg.mozilla.org/mozilla-central/rev/f7cf60910637
Comment 10 :Ehsan Akhgari (out sick) 2012-08-16 11:39:21 PDT
There are no noticeable changes in the linker vmem size or codesighs.
Comment 11 :Ehsan Akhgari (out sick) 2012-08-16 13:55:23 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.