> -----Original Message----- > From: dev [mailto:dev-boun...@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 > <m...@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
