1585585 - Add MOZ_UNLIKELY to if branch in MOZ_TRY macro

Status: RESOLVED FIXED

(Core :: MFBT, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/f372e8a46ef7659ef61be9938ec2a3ea34d343c6/mfbt/Result.h#502-508

#define MOZ_TRY(expr)                                       \
  do {                                                      \
    auto mozTryTempResult_ = ::mozilla::ToResult(expr);     \
    if (mozTryTempResult_.isErr()) {                        \
      return ::mozilla::Err(mozTryTempResult_.unwrapErr()); \
    }                                                       \
  } while (0)

IMO, the isErr() case is almost always error case and not-hot code path.
adding MOZ_UNLIKELY should improve the generated code.

in BinAST parser (that heavily used MOZ_TRY macro and variants), adding MOZ_UNLIKELY notably improves the performance (~3% speed up with parsing 200 files)

Comment 1

[(Away)]

Impressive!

Maybe we should also add it to other cases in Result.h, like all those isOk() ? x : y lines.

Comment 2

[Jeff Walden [:Waldo]]

Or perhaps isOk() itself could return MOZ_LIKELY(...) instead of .... It does not seem unreasonable to me to impose upon Result the intended semantic that the success case be normal and errors be irregular.

Comment 3

[Jeff Walden [:Waldo]]

(And of course isErr() would return MOZ_UNLIKELY(...), and whatever other relevant functions that could use similar treatment would get it.)

Comment 4

[Mike Hommey [:glandium]]

(In reply to Tooru Fujisawa [:arai] from comment #0)

in BinAST parser (that heavily used MOZ_TRY macro and variants), adding MOZ_UNLIKELY notably improves the performance (~3% speed up with parsing 200 files)

That'd be a case for adding something that uses the BinAST parser to the PGO profile.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1585585 - Add {MOZ_LIKELY,MOZ_UNLIKELY} to Result::{isOk,isErr}. r?jwalden

Depends on D48021

Comment 6

[Chris Peterson [:cpeterson]]

bool isOk() const { return MOZ_LIKELY(mImpl.isOk()); }
...
bool isErr() const { return MOZ_UNLIKELY(!mImpl.isOk()); }

Does clang know how to optimize the caller code when a function returns MOZ_LIKELY or MOZ_UNLIKELY? Do you still see the same ~3% speed up as you do when using MOZ_UNLIKELY in the MOZ_TRY macro directly?

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Chris Peterson [:cpeterson] from comment #6)

bool isOk() const { return MOZ_LIKELY(mImpl.isOk()); }
...
bool isErr() const { return MOZ_UNLIKELY(!mImpl.isOk()); }

Does clang know how to optimize the caller code when a function returns MOZ_LIKELY or MOZ_UNLIKELY? Do you still see the same ~3% speed up as you do when using MOZ_UNLIKELY in the MOZ_TRY macro directly?

thanks for pointing out. you're right. it doesn't work as I expect.

will post fixed one.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1585585 - Add {MOZ_LIKELY,MOZ_UNLIKELY} to Result::{isOk,isErr} consumers. r?jwalden

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01862196080a
Add {MOZ_LIKELY,MOZ_UNLIKELY} to Result::{isOk,isErr} consumers. r=jwalden

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/01862196080a