[dpdk-dev] rte_mbuf library likely()/unlikely()
Morten Brørup
mb at smartsharesystems.com
Tue Jul 24 10:13:43 CEST 2018
> -----Original Message-----
> From: dev [mailto:dev-bounces at 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 at 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
More information about the dev
mailing list