Last Comment Bug 800106 - Replace usages of NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE and remove NS_ALWAYS_INLINE
: Replace usages of NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE and remove NS_ALWAY...
Status: RESOLVED FIXED
[mentor=ehsan][lang=c++]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: maligree
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-10 13:00 PDT by :Ehsan Akhgari
Modified: 2012-11-07 17:30 PST (History)
13 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to replace NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE across tree. (10.36 KB, patch)
2012-10-12 23:38 PDT, maligree
ehsan: feedback+
Details | Diff | Splinter Review
Second version of patch to replace NS_ALWAYS_INLINE. (12.90 KB, patch)
2012-10-13 09:09 PDT, maligree
ehsan: review+
Details | Diff | Splinter Review
no libjpeg & no mozalloc.h changes (7.56 KB, patch)
2012-10-19 01:42 PDT, maligree
ehsan: checkin+
Details | Diff | Splinter Review
libjpeg modified, no mozalloc changes (7.93 KB, patch)
2012-10-19 01:43 PDT, maligree
no flags Details | Diff | Splinter Review
nsUnicharUtils.cpp & StatementCache.h (2.68 KB, patch)
2012-10-23 04:34 PDT, maligree
ehsan: checkin+
Details | Diff | Splinter Review
SQLFunctions.cpp & nsUTF8Utils.h (5.08 KB, patch)
2012-10-23 04:35 PDT, maligree
ehsan: checkin+
Details | Diff | Splinter Review
Drop inline annotations in nsUTF8Utils.h (2.69 KB, patch)
2012-10-24 17:32 PDT, :Ehsan Akhgari
dbaron: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-10-10 13:00:42 PDT
The idea is to remove the NS_ALWAYS_INLINE definition from configure.in and replace the rest of the instances with MOZ_ALWAYS_INLINE.

http://mxr.mozilla.org/mozilla-central/search?string=NS_ALWAYS_INLINE
Comment 1 Daniel Holbert [:dholbert] 2012-10-10 13:02:37 PDT
yes please.
Comment 2 maligree 2012-10-12 23:38:54 PDT
Created attachment 671038 [details] [diff] [review]
Patch to replace NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE across tree.

How does this look?

I'm not sure about adding the #include to libjpeg/config.h: is this right?
Comment 3 :Ehsan Akhgari 2012-10-13 05:38:36 PDT
Comment on attachment 671038 [details] [diff] [review]
Patch to replace NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE across tree.

Review of attachment 671038 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  Please address the comments below and this will be ready for review and landing.  Thanks!

::: configure.in
@@ -3841,5 @@
>  dnl are defined in build/autoconf/altoptions.m4.
>  
>  dnl If the compiler supports these attributes, define them as
>  dnl convenience macros.
> -if test "$ac_cv_attribute_always_inline" = yes ; then

You can probably remove the definition of ac_cv_attribute_always_inline earlier in this file too.

::: js/src/configure.in
@@ -3151,5 @@
>  dnl are defined in build/autoconf/altoptions.m4.
>  
>  dnl If the compiler supports these attributes, define them as
>  dnl convenience macros.
> -if test "$ac_cv_attribute_always_inline" = yes ; then

Here as well.

::: media/libjpeg/config.h
@@ +3,5 @@
>  #define PACKAGE_NAME "libjpeg-turbo"
>  
>  /* Need to use Mozilla-specific function inlining. */
> +#include "mozilla/Attributes.h"
> +#define INLINE MOZ_ALWAYS_INLINE

This should be fine.  Please edit media/libjpeg/MOZCHANGES as well.

::: memory/mozalloc/mozalloc.h
@@ +39,5 @@
>  #endif
>  
>  
> +#if defined(MOZ_ALWAYS_INLINE)
> +#  define MOZALLOC_INLINE MOZ_ALWAYS_INLINE inline

Please drop `inline' here.  MOZ_ALWAYS_INLINE should include that.
Comment 4 :Ehsan Akhgari 2012-10-13 05:39:05 PDT
Oh, and thanks a lot for your patch!  :-)
Comment 5 maligree 2012-10-13 09:09:08 PDT
Created attachment 671101 [details] [diff] [review]
Second version of patch to replace NS_ALWAYS_INLINE.

Updated as per comment 3. ;-)
Comment 6 :Ehsan Akhgari 2012-10-13 10:30:02 PDT
Comment on attachment 671101 [details] [diff] [review]
Second version of patch to replace NS_ALWAYS_INLINE.

Review of attachment 671101 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!
Comment 7 :Ehsan Akhgari 2012-10-13 10:32:53 PDT
Submitted to the try server to make sure that it successfully builds on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=661a4cd0c975
Comment 8 Mozilla RelEng Bot 2012-10-13 12:45:36 PDT
Try run for 661a4cd0c975 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=661a4cd0c975
Results (out of 16 total builds):
    success: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-661a4cd0c975
Comment 9 :Ehsan Akhgari 2012-10-13 13:41:46 PDT
Try results were green, pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/0f0797cdb55a
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-10-13 13:47:35 PDT
Comment on attachment 671101 [details] [diff] [review]
Second version of patch to replace NS_ALWAYS_INLINE.

Review of attachment 671101 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/mozalloc/mozalloc.h
@@ +39,5 @@
>  #endif
>  
>  
> +#if defined(MOZ_ALWAYS_INLINE)
> +#  define MOZALLOC_INLINE MOZ_ALWAYS_INLINE

This may cause issues in debug builds; see bug 715093.
Comment 11 maligree 2012-10-14 04:40:31 PDT
So Talos says XP got a bit slower with MOZ_ALWAYS_INLINE:

Regression :( Kraken Benchmark increase 9.19% on XP Mozilla-Inbound-Non-PGO 
--------------------------------------------------------------------------- 
    Previous: avg 3644.030 stddev 5.593 of 30 runs up to revision fef8303baf67 
    New     : avg 3978.760 stddev 21.101 of 5 runs since revision 0f0797cdb55a 
    Change  : +334.730 (9.19% / z=59.848) 
    Graph   : http://mzl.la/Oybx5Y

What's the modus operandi with this?

On an unrelated and rather educational note, can anyone tell me why do debug builds resort to a pure, un-forced inline?

#if defined(DEBUG)
#  define MOZ_ALWAYS_INLINE     MOZ_INLINE

http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#40
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-10-14 14:09:10 PDT
https://hg.mozilla.org/mozilla-central/rev/0f0797cdb55a
Comment 13 :Ehsan Akhgari 2012-10-15 10:51:21 PDT
(In reply to :Ms2ger from comment #10)
> Comment on attachment 671101 [details] [diff] [review]
> Second version of patch to replace NS_ALWAYS_INLINE.
> 
> Review of attachment 671101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/mozalloc/mozalloc.h
> @@ +39,5 @@
> >  #endif
> >  
> >  
> > +#if defined(MOZ_ALWAYS_INLINE)
> > +#  define MOZALLOC_INLINE MOZ_ALWAYS_INLINE
> 
> This may cause issues in debug builds; see bug 715093.

Hmm, I'm not sure why.  Doesn't this guarantee those functions to be inline?
Comment 14 :Ehsan Akhgari 2012-10-15 11:12:46 PDT
(In reply to maligree from comment #11)
> So Talos says XP got a bit slower with MOZ_ALWAYS_INLINE:
> 
> Regression :( Kraken Benchmark increase 9.19% on XP Mozilla-Inbound-Non-PGO 
> --------------------------------------------------------------------------- 
>     Previous: avg 3644.030 stddev 5.593 of 30 runs up to revision
> fef8303baf67 
>     New     : avg 3978.760 stddev 21.101 of 5 runs since revision
> 0f0797cdb55a 
>     Change  : +334.730 (9.19% / z=59.848) 
>     Graph   : http://mzl.la/Oybx5Y
> 
> What's the modus operandi with this?

Ouch.  I think we'll need to back out this patch.  :(

Can you please try applying individual pieces of this patch in the bug to see which one regresses the performance?

> On an unrelated and rather educational note, can anyone tell me why do debug
> builds resort to a pure, un-forced inline?
> 
> #if defined(DEBUG)
> #  define MOZ_ALWAYS_INLINE     MOZ_INLINE
> 
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#40

I have no idea!  Waldo?
Comment 15 :Ehsan Akhgari 2012-10-15 11:21:04 PDT
Backed out :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f27ce421c8f
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-15 11:40:23 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Hmm, I'm not sure why.  Doesn't this guarantee those functions to be inline?

MOZ_ALWAYS_INLINE is kind of misnomer-ish, because there's no way in standard C++ to force a function to be inlined; the macro just tries harder to inline the function, using compiler-specific hacks wherever possible, and in the end it falls back to just "inline".  MOZ_ALWAYS_INLINE's documentation:

/*
 * MOZ_ALWAYS_INLINE is a macro which expands to tell the compiler that the
 * method decorated with it must be inlined, even if the compiler thinks
 * otherwise.  This is only a (much) stronger version of the MOZ_INLINE hint:
 * compilers are not guaranteed to respect it (although they're much more likely
 * to do so).
 */

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> > On an unrelated and rather educational note, can anyone tell me why do debug
> > builds resort to a pure, un-forced inline?
> > 
> > #if defined(DEBUG)
> > #  define MOZ_ALWAYS_INLINE     MOZ_INLINE
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#40
> 
> I have no idea!  Waldo?

No idea either.

Semi-unrelatedly, since MOZ_ALWAYS_INLINE is always defined, the ifdef dance currently used there is pointless -- only the first consequent is ever used.  And in that case, MOZALLOC_INLINE should just be removed in favor of MOZ_ALWAYS_INLINE.  (Assuming the perf issue or whatever is unrelated.)
Comment 17 Ed Morley [:emorley] 2012-10-16 01:12:30 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> Backed out :(
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5f27ce421c8f

https://hg.mozilla.org/mozilla-central/rev/5f27ce421c8f
Comment 18 maligree 2012-10-16 07:16:40 PDT
> Can you please try applying individual pieces of this patch in the bug to
> see which one regresses the performance?

Hm, don't I need access to the try server to do that? I could always split my patch into parts and you'd push, I guess.
Comment 19 maligree 2012-10-16 07:50:55 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Semi-unrelatedly, since MOZ_ALWAYS_INLINE is always defined, the ifdef dance
> currently used there is pointless -- only the first consequent is ever used.
> And in that case, MOZALLOC_INLINE should just be removed in favor of
> MOZ_ALWAYS_INLINE.  (Assuming the perf issue or whatever is unrelated.)

Didn't notice that before. Looks like this leaves FORCE_INLINE completely unused (http://mxr.mozilla.org/mozilla-central/search?string=HAVE_FORCEINLINE) and could probably be removed as well?
Comment 20 maligree 2012-10-16 07:54:55 PDT
I also wonder what was the purpose of the explicit `inline' here:

#if defined(NS_ALWAYS_INLINE)
#  define MOZALLOC_INLINE NS_ALWAYS_INLINE inline

(And, though probably off-topic here how do compilers behave when faced with a combination of inline, __inline & __forcedinline)

Well, this turned out to be quite interesting. Frustrating, but interesting. ;-)
Comment 21 :Ehsan Akhgari 2012-10-16 17:16:50 PDT
(In reply to comment #18)
> > Can you please try applying individual pieces of this patch in the bug to
> > see which one regresses the performance?
> 
> Hm, don't I need access to the try server to do that? I could always split my
> patch into parts and you'd push, I guess.

You do, and you can apply for that and I'll happily vouch for you! See <http://www.mozilla.org/hacking/commit-access-policy/>.

But honestly measuring performance on the try server is a big pain, so if you want to attach individual patches I will be happy to land them for you piecemeal so that we can catch any individual regressions.
Comment 22 :Ehsan Akhgari 2012-10-16 17:17:04 PDT
(In reply to comment #19)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> > Semi-unrelatedly, since MOZ_ALWAYS_INLINE is always defined, the ifdef dance
> > currently used there is pointless -- only the first consequent is ever used.
> > And in that case, MOZALLOC_INLINE should just be removed in favor of
> > MOZ_ALWAYS_INLINE.  (Assuming the perf issue or whatever is unrelated.)
> 
> Didn't notice that before. Looks like this leaves FORCE_INLINE completely
> unused (http://mxr.mozilla.org/mozilla-central/search?string=HAVE_FORCEINLINE)
> and could probably be removed as well?

Sure, after all of the patches here have landed.
Comment 23 :Ehsan Akhgari 2012-10-16 17:19:59 PDT
(In reply to comment #20)
> I also wonder what was the purpose of the explicit `inline' here:

Compilers mostly ignore 'inline' these days.  It's pretty much the same as whitespace on all real compilers.  ;-)

> #if defined(NS_ALWAYS_INLINE)
> #  define MOZALLOC_INLINE NS_ALWAYS_INLINE inline
> 
> (And, though probably off-topic here how do compilers behave when faced with a
> combination of inline, __inline & __forcedinline)

See <http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx>.

> Well, this turned out to be quite interesting. Frustrating, but interesting.
> ;-)

Indeed. Usually first patches are not this difficult to get into the tree!  :-)  I'm very happy that you were not disappointed and are still sticking around!
Comment 24 maligree 2012-10-17 03:13:50 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #21)
> You do, and you can apply for that and I'll happily vouch for you! See
> <http://www.mozilla.org/hacking/commit-access-policy/>.

That's awesome! https://bugzilla.mozilla.org/show_bug.cgi?id=802537 ;-)
Comment 25 maligree 2012-10-19 01:42:48 PDT
Created attachment 673136 [details] [diff] [review]
no libjpeg & no mozalloc.h changes
Comment 26 maligree 2012-10-19 01:43:32 PDT
Created attachment 673137 [details] [diff] [review]
libjpeg modified, no mozalloc changes
Comment 27 maligree 2012-10-19 01:50:28 PDT
Ehsan: could you try these two patches?

- v4 replaces all occurences of NS_ALWAYS_INLINE except those in libjpeg and mozalloc
- v5 replaces all occurences of NS_ALWAYS_INLINE except those in mozalloc

These are sort of blind guesses (not really a fan), but perhaps it's worth a try? 

I have other (v1-v3, obviously) patches ready - which touch even fewer files - but I figured libjpeg and mozalloc would be our main suspects.

Oh, and sorry - had to take care of some boring worldly matters, hence the delay. ;)
Comment 28 maligree 2012-10-19 01:54:21 PDT
> - v4 replaces all occurences of NS_ALWAYS_INLINE except those in libjpeg and
> mozalloc
> - v5 replaces all occurences of NS_ALWAYS_INLINE except those in mozalloc

My totally rad versioning scheme got lost in the process of submitting and they're all named *-v1, but I hope you'll figure it out. Sorry.
Comment 29 :Ehsan Akhgari 2012-10-19 08:37:52 PDT
Comment on attachment 673136 [details] [diff] [review]
no libjpeg & no mozalloc.h changes

Thanks Jacek, I pushed this patch for now, so let's wait and see what happens!

https://hg.mozilla.org/integration/mozilla-inbound/rev/788a51ef6221
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-10-19 19:07:16 PDT
https://hg.mozilla.org/mozilla-central/rev/788a51ef6221
Comment 31 :Ehsan Akhgari 2012-10-22 13:44:40 PDT
The part landed in comment 29 caused another regression:

Regression  Kraken Benchmark increase 10.1% on XP Mozilla-Inbound-Non-PGO
---------------------------------------------------------------------------
    Previous: avg 3641.293 stddev 9.805 of 30 runs up to revision 0cc577753007
    New     : avg 4009.480 stddev 3.484 of 5 runs since revision 788a51ef6221
    Change  : +368.187 (10.1% / z=37.552)
    Graph   : http://mzl.la/QwPqvn

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0cc577753007&tochange=788a51ef6221

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/788a51ef6221
    : Jacek Szpot <maligree@gmail.com> - Bug 800106: replace some NS_ALWAYS_INLINEs with MOZ_ALWAYS_INLINEs; r=ehsan
    : http://bugzilla.mozilla.org/show_bug.cgi?id=800106

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=800106 - Replace usages of NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE and remove NS_ALWAYS_INLINE

So the offending thing is in this part of the patch.  I've backed it out for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f09b604856

Jacek, can you split this patch up further?
Comment 32 :Ehsan Akhgari 2012-10-22 13:46:07 PDT
(FWIW I doubt that the places stuff is to blame here.)
Comment 33 maligree 2012-10-23 04:34:33 PDT
Created attachment 674200 [details] [diff] [review]
nsUnicharUtils.cpp & StatementCache.h
Comment 34 maligree 2012-10-23 04:35:23 PDT
Created attachment 674201 [details] [diff] [review]
SQLFunctions.cpp & nsUTF8Utils.h
Comment 35 :Ehsan Akhgari 2012-10-23 07:58:53 PDT
Comment on attachment 674200 [details] [diff] [review]
nsUnicharUtils.cpp & StatementCache.h

Thanks!  Landed the first part:

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ce8e5bd309

I'll land the second part later today, after this first part manages to get some testing.
Comment 36 :Ehsan Akhgari 2012-10-23 13:25:11 PDT
Comment on attachment 674201 [details] [diff] [review]
SQLFunctions.cpp & nsUTF8Utils.h

Landed this part as well:

https://hg.mozilla.org/integration/mozilla-inbound/rev/531b7f4293e9
Comment 37 :Ehsan Akhgari 2012-10-23 18:11:54 PDT
So it turns out that http://hg.mozilla.org/integration/mozilla-inbound/rev/531b7f4293e9 regressed the Kraken on XP!  That patch has only two parts, the places one and the nsUTF8Utils.h one, and I am going to bet some serious money that the regression is coming from the nsUTF8Utils.h change!  I've backed out that part of the patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f88c599f95a and will watch the Kraken numbers to make sure that they go back up.  Once they do, I'll land the rest of the patch.  Looks like we've got our culprit nailed!  :-)
Comment 38 :Ehsan Akhgari 2012-10-23 18:21:31 PDT
So I digged back through our code history to figure out when we introduced the NS_ALWAYS_INLINEs in nsUTF8Utils.h.  It goes back to this commit <https://github.com/mozilla/mozilla-central/commit/fe123b96af76fdcba4d584f8bc30a4317cfdb03c> from bug 206682, authored about 9 years ago by dbaron.

NS_ALWAYS_INLINE was not used on non-gcc, which means that the patch here does in fact change the behavior.  These functions can be rather large, and it's not entirely surprising to me that force-inlining them can have performance implications.  Also, note that we have never seen this regression for our PGO builds, which suggests to me that with PGO, the compiler just ignores us and does the right thing, which is good.

Given the above, I believe that we should just drop the NS_ALWAYS_INLINEs there (and of course make sure that it doesn't regress our Linux and Mac builds.  CCing the String peers to solicit their opinion.
Comment 40 Justin Lebar (not reading bugmail) 2012-10-23 22:12:34 PDT
> Also, note that we have never seen this regression for our PGO builds, which suggests to me that 
> with PGO, the compiler just ignores us and does the right thing, which is good.

So to be clear, we regressed Kraken on non-PGO Windows builds?

> Given the above, I believe that we should just drop the NS_ALWAYS_INLINEs [in nsUTF8Utils.h] (and of 
> course make sure that it doesn't regress our Linux and Mac builds).

That seems reasonable, given what we know.  But of course we need to understand how the change affects the other platforms.
Comment 41 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-10-24 00:12:49 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #38)
> So I digged back through our code history to figure out when we introduced
> the NS_ALWAYS_INLINEs in nsUTF8Utils.h.  It goes back to this commit
> <https://github.com/mozilla/mozilla-central/commit/
> fe123b96af76fdcba4d584f8bc30a4317cfdb03c> from bug 206682, authored about 9
> years ago by dbaron.
> 
> NS_ALWAYS_INLINE was not used on non-gcc, which means that the patch here
> does in fact change the behavior.  These functions can be rather large, and
> it's not entirely surprising to me that force-inlining them can have
> performance implications.  Also, note that we have never seen this
> regression for our PGO builds, which suggests to me that with PGO, the
> compiler just ignores us and does the right thing, which is good.
> 
> Given the above, I believe that we should just drop the NS_ALWAYS_INLINEs
> there (and of course make sure that it doesn't regress our Linux and Mac
> builds.  CCing the String peers to solicit their opinion.

So my understanding is that the NS_ALWAYS_INLINE functions in nsUTF8Utils.h were only used in one place, so there shouldn't be any harm in forcing them to be inlined.  (It's possible that's changed, though, but I don't think it should have.)  Furthermore, the performance of the character sink pattern that they use (for the shared copy_string function) sort of depends on their being inlined.  I wouldn't expect forcing them to be inlined on additional platforms to be harmful.  At the time I wrote it it was needed for gcc, and I didn't bother figuring out how to do it for other platforms.
Comment 42 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-10-24 00:17:39 PDT
I'd be curious to know how adding the MOZ_ALWAYS_INLINE changed the generated code on Windows.  What made it slower?  Was it generation of more code?  Did it cause something else not to be inlined?

(Also, when you ask me questions in bugs, I'll see it faster if you use needinfo?.)
Comment 43 :Ehsan Akhgari 2012-10-24 06:31:34 PDT
Landed the rest of the parts of this patch which do not cause regressions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae1d0d7b33c

I'll look at the details of the generated code later.
Comment 44 Ryan VanderMeulen [:RyanVM] 2012-10-24 11:10:03 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #37)
> I've backed out that part of the patch:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7f88c599f95a

https://hg.mozilla.org/mozilla-central/rev/7f88c599f95a
Comment 45 :Ehsan Akhgari 2012-10-24 17:16:02 PDT
So I looked more closely into the generated code on Windows.  Here is the generated code on Windows with NS_ALWAYS_INLINE (or without any inline specifier since they're practically the same:

https://gist.github.com/3949724

In short, copy_string and nsCharSinkTraits::write end up being inlined in AppendUTF8toUTF16, and that function just directly calls ConvertUTF8toUTF16::write, which is not inlined.

When we force-inline ConvertUTF8toUTF16::write, however, presumably the compiler can't do the first two inlinings because of the total cost of inlining, so it ends up generating the following code:

https://gist.github.com/3949677

Here, ConvertUTF8toUTF16::write gets inlined in nsCharSinkTraits::write, and nsCharSinkTraits::write is called from copy_string, and copy_string gets called from AppendUTF8toUTF16, so we have the overhead of the two function calls added.

I didn't look at the code that the PGO compiler generates because it would take ages to do two PGO builds, but I think it's fair to assume that PGO just cleans up after our mess when we end up using MOZ_ALWAYS_INLINE.

This is a good example why it's not a good idea for us mere mortals to interfere with the compiler's inling decisions.  ;-)  And I think this is good evidence in favor of dropping NS_ALWAYS_INLINEs there.

What do you think, David?
Comment 46 :Ehsan Akhgari 2012-10-24 17:32:23 PDT
Created attachment 674914 [details] [diff] [review]
Drop inline annotations in nsUTF8Utils.h
Comment 47 Ryan VanderMeulen [:RyanVM] 2012-10-24 19:17:22 PDT
https://hg.mozilla.org/mozilla-central/rev/6ae1d0d7b33c
Comment 48 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-11-07 11:05:48 PST
Comment on attachment 674914 [details] [diff] [review]
Drop inline annotations in nsUTF8Utils.h

ok, r=dbaron

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