Re: [dpdk-dev] rte_mbuf library likely()/unlikely()

["Morten Brørup" <mb@smartsharesystems.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Tuesday, July 24, 2018 9:29 AM
> 
> Hi,
> 
> On Mon, Jul 23, 2018 at 10:40:03PM +0000, Wiles, Keith wrote:
> >
> >
> > > On Jul 23, 2018, at 2:09 PM, Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > >
> > > I haven't performance tested, but they are compiler branch
> prediction hints pointing out the most likely execution path, so I
> expect them to have a positive effect.
> >
> > We really need to make sure this provides any performance improvement
> and that means it needs to be tested on a number of systems. Can you
> please do some performance testing or see if we can get the guys doing
> DPDK performance testing to first give this a try? This area is very
> sensitive to tweaking.
> 
> I agree we should be driven by performance improvements.

Which is why I suggested these changes. Theoretically, they will provide a performance improvement. The most likely execution path is obvious from code review.

> I remember a discussion with Bruce on the ML saying that hardware
> branch
> predictors generally do a good job.

They do, and it is very well documented. E.g. here's a really interesting historical review about branch predictors:
https://danluu.com/branch-prediction/

However, just because hardware branch predictors are pretty good, I don't think we should remove or stop adding likely()/unlikely() and other branch prediction hints. The hints still add value, both for execution speed and for source code readability.

Please also refer to the other link I provided about GCC branches. It basically says that GCC treats an If-sentence like this:

If (Condition) Then
	Expect to execute this
Else
	Do not expect to execute this

So if we don't want unlikely() around an if-condition which probably evaluates to false, we should rewrite the execution order accordingly.

Although hardware branch predictors help a lot most of the time, the likely()/unlikely() still helps the first time the CPU executes the branch instruction.

Furthermore, I'm very well aware of the rule of thumb for adding likely()/unlikely(): Don't add one if it isn't correct almost every time the branch is considered.

How much more compiler branch prediction hints adds to hardware compiler branch prediction is a somewhat academic discussion. But Honnappa and Keith are right: Performance improvements should be performance tested.

Unfortunately, I don't have the equipment or resources to perform a usable performance test, so I submitted the changes to the mailing list for code review instead. And I'm certainly getting code reviewed now. :-)

From a code review perspective, someone else than me might observe that the exception handling execution path is "missing" the unlikely() hint, so I would argue that code readability is an argument for adding it - unless performance testing shows a slowdown.


Med venlig hilsen / kind regards
- Morten Brørup