792180 - Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Currently, nscore.h [1] has NS_LIKELY and NS_UNLIKELY. MOZ_LIKELY and MOZ_UNLIKELY in mozilla/Likely.h [2] are better alternatives. The purpose of this bug is to

* Replace all occurrences of NS_LIKELY and NS_UNLIKELY [3,4] with the MOZ_* variants;
* Add #include "mozilla/Likely.h" to all touched files;
* Remove the definitions of NS_LIKELY and NS_UNLIKELY from nscore.h

Since there are a few hundred occurrences of these macros, it would probably be easiest to write a script to automate the first part. You could use [5] for inspiration.

Feel free to ask me if you have any questions.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#366
[2] http://mxr.mozilla.org/mozilla-central/source/mfbt/Likely.h
[3] http://mxr.mozilla.org/mozilla-central/ident?i=NS_LIKELY
[4] http://mxr.mozilla.org/mozilla-central/ident?i=NS_UNLIKELY
[5] https://bug579517.bugzilla.mozilla.org/attachment.cgi?id=653946

Comment 1

[Mats Palmgren (inactive)]

> MOZ_LIKELY and MOZ_UNLIKELY in mozilla/Likely.h [2] are better alternatives.

No, they are worse alternatives.  They fail to compile:
  if (MOZ_LIKELY(f))  // f is a nsIFrame*

(see bug 340244 for details)

I do agree with the naming change though, but please change the
MOZ_ macros to match the NS_ ones first.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Mats Palmgren [:mats] from comment #1)
> > MOZ_LIKELY and MOZ_UNLIKELY in mozilla/Likely.h [2] are better alternatives.
> 
> No, they are worse alternatives.  They fail to compile:
>   if (MOZ_LIKELY(f))  // f is a nsIFrame*
> 
> (see bug 340244 for details)
> 
> I do agree with the naming change though, but please change the
> MOZ_ macros to match the NS_ ones first.

This has been fixed.

Comment 3

[tienthanh8490]

Hi Ms2ger, i would like to help on this bug. Could you please assist me with this ? Thank you.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to tienthanh8490 from comment #3)
> Hi Ms2ger, i would like to help on this bug. Could you please assist me with
> this ? Thank you.

Certainly. If you aren't yet, it's probably easiest to get on IRC (<https://wiki.mozilla.org/IRC>), and join the #introduction channel.

Comment 5

[chronon]

tienthanh8490, are you still working on this bug? If not, I'd be happy to take it over

Comment 6

[maligree]

Attachment: Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY across tree

It was a slow day today, so I figured..

And it even builds.

Comment 7

[tienthanh8490]

(In reply to chronon from comment #5)
> tienthanh8490, are you still working on this bug? If not, I'd be happy to
> take it over

Yes i dropped out, please take it over

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 675285 [details] [diff] [review]
Replace NS_{UN,}LIKELY with MOZ_{UN,}LIKELY across tree

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

Thanks a lot! I made some small comments below; please fix those and ask :ehsan to review.

Henri, could you check if the changes here touch generated code?

::: accessible/src/atk/nsMaiInterfaceAction.cpp
@@ +8,5 @@
>  
>  #include "Accessible-inl.h"
>  #include "nsMai.h"
>  #include "Role.h"
> +#include "mozilla/Likely.h" 

Remove the extra space at the end of this line, here and elsewhere in the patch.

::: content/base/src/nsINode.cpp
@@ +14,5 @@
>  #include "mozAutoDocUpdate.h"
>  #include "mozilla/CORSMode.h"
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Put this one between CORSMode and Telemetry, to preserve the alphabetical ordering.

::: gfx/thebes/gfxFont.cpp
@@ +33,5 @@
>  #include "mozilla/FloatingPoint.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/Services.h"
>  #include "mozilla/Telemetry.h"
> +#include "mozilla/Likely.h"

Ordering again.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +17,5 @@
>  #include "mozilla/Preferences.h"
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/Likely.h"

Sort those too

::: js/xpconnect/src/nsXPConnect.cpp
@@ +8,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Base64.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Again.

::: layout/base/nsPresShell.cpp
@@ +20,5 @@
>  
>  #include "mozilla/dom/PBrowserChild.h"
>  #include "mozilla/dom/TabChild.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Here

::: layout/generic/nsTextFrameThebes.cpp
@@ +73,5 @@
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/Util.h" // for DebugOnly
>  #include "mozilla/LookAndFeel.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/Likely.h"

Sort those too, please.

::: layout/style/nsRuleNode.cpp
@@ +43,5 @@
>  #include "mozilla/Assertions.h"
>  #include "mozilla/dom/Element.h"
>  #include "mozilla/LookAndFeel.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Likely.h"

Sort

::: layout/xul/base/src/nsMenuFrame.cpp
@@ +40,5 @@
>  #include "mozilla/Services.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/LookAndFeel.h"
>  #include "mozilla/Attributes.h"
> +#include "mozilla/Likely.h"

And here, please.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +571,1 @@
>                               NS_ERROR_HTMLPARSER_INTERRUPTED)) {

Fix the indentation here

::: toolkit/xre/nsAppRunner.cpp
@@ +1061,5 @@
>  #endif
>  
>  #define NS_ENSURE_TRUE_LOG(x, ret)               \
>    PR_BEGIN_MACRO                                 \
> +  if (MOZ_UNLIKELY(!(x))) {                       \

Fix the alignment of the backslash

::: xpcom/base/nsCycleCollector.cpp
@@ +140,5 @@
>  #include "mozilla/Mutex.h"
>  #include "mozilla/CondVar.h"
>  #include "mozilla/StandardInteger.h"
>  #include "mozilla/Telemetry.h"
> +#include "mozilla/Likely.h" 

Sort, please

Comment 9

[maligree]

Attachment: Fixed ordering, indentation, and removed trailing whitespaces

Comment 10

[maligree]

Oops, shouldn't have marked the previous one as obsolete, I guess - looks like I cleared hsivonen's review? flag..? Sorry.

Comment 11

[Henri Sivonen (:hsivonen)]

r+ for the generated parser files from me. There are indeed generated files being edited, but I can easily change the generator to match once this has landed.

Comment 12

[(no longer active)]

I'll try to review this over the weekend.  Sorry for the delay!

Comment 13

[(no longer active)]

Comment on attachment 675534 [details] [diff] [review]
Fixed ordering, indentation, and removed trailing whitespaces

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

Looks great, thanks!

::: layout/style/nsRuleNode.cpp
@@ +2320,5 @@
>      else                                                                      \
>        data_ = new (mPresContext) nsStyle##type_ ctorargs_;                    \
>    }                                                                           \
>                                                                                \
> +  if (MOZ_UNLIKELY(!data_))                                                    \

There's a bunch of places where this patch breaks indentation but I can fix those when landing.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd96dda75f0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bd96dda75f0