Closed Bug 800106 Opened 12 years ago Closed 12 years ago

Replace usages of NS_ALWAYS_INLINE with MOZ_ALWAYS_INLINE and remove NS_ALWAYS_INLINE

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19

People

(Reporter: ehsan.akhgari, Assigned: maligree)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(4 files, 3 obsolete files)

Second version of patch to replace NS_ALWAYS_INLINE.
12.90 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
nsUnicharUtils.cpp & StatementCache.h
2.68 KB, patch
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
SQLFunctions.cpp & nsUTF8Utils.h
5.08 KB, patch
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
Drop inline annotations in nsUTF8Utils.h
2.69 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
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
yes please.
How does this look?

I'm not sure about adding the #include to libjpeg/config.h: is this right?
Attachment #671038 - Flags: feedback?(ehsan)
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.
Attachment #671038 - Flags: feedback?(ehsan) → feedback+
Oh, and thanks a lot for your patch!  :-)
Updated as per comment 3. ;-)
Attachment #671038 - Attachment is obsolete: true
Attachment #671101 - Flags: review?(ehsan)
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!
Attachment #671101 - Flags: review?(ehsan) → review+
Submitted to the try server to make sure that it successfully builds on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=661a4cd0c975
Assignee: nobody → maligree
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
Try results were green, pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/0f0797cdb55a
Target Milestone: --- → mozilla19
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.
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
https://hg.mozilla.org/mozilla-central/rev/0f0797cdb55a
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(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?
(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?
Backed out :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f27ce421c8f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.)
> 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.
(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?
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. ;-)
(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.
(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.
(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!
(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 ;-)

    
    

    
  


  

    
    

    
  
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. ;)

    
    

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

        Attachment #673136 -
        Flags: checkin+

    
  
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][leave open]

    
    

    
  
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?

    
    

    
  
(FWIW I doubt that the places stuff is to blame here.)

    
    

    
  


  

        Attachment #673136 -
        Attachment is obsolete: true

        Attachment #673137 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

    
  
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.

        Attachment #674200 -
        Flags: checkin+

    
    

    
  
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

        Attachment #674201 -
        Flags: checkin+

    
    

    
  
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!  :-)

    
    

    
  
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.

    
    

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

    
    

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

    
    

    
  
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?.)

    
    

    
  
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.

    
    

    
  
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?
Flags: needinfo?(dbaron)

    
    

    
  


  

        Attachment #674914 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 674914 [details] [diff] [review]
Drop inline annotations in nsUTF8Utils.h

ok, r=dbaron

        Attachment #674914 -
        Flags: review?(dbaron) → review+
Flags: needinfo?(dbaron)

    
    

    
  
Landed the two final pieces:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad737ac36a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc13c881fdf5
Whiteboard: [mentor=ehsan][lang=c++][leave open] → [mentor=ehsan][lang=c++]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3ad737ac36a4
https://hg.mozilla.org/mozilla-central/rev/dc13c881fdf5
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: