| Message ID | 20230903095059.2683850-1-visitorckw@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | md/raid5: eliminate if-statements in cmp_stripe() | expand |
On Sun, 3 Sep 2023 17:50:59 +0800 Kuan-Wei Chiu <visitorckw@gmail.com> wrote: > Replace the conditional statements in the cmp_stripe() function with a > branchless version to improve code readability and potentially enhance > performance. The new code will always do two comparisons and a subtraction (3 instructions in total), whereas the old version could return after just 1 comparison, or after 2 comparisons. So depending on the data values it is 3x to 1.5x as much operations performed than before, there unlikely to be any enhancement of performance. Also IMO the previous version is more easily readable. > The new code calculates the result using a subtraction of > comparison results, making it more concise and avoiding conditional > branches. This change does not alter the functionality of the code. > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > --- > drivers/md/raid5.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index 4cb9c608ee19..b14d7ba38f0f 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -1035,11 +1035,7 @@ static int cmp_stripe(void *priv, const struct list_head *a, > struct r5pending_data, sibling); > const struct r5pending_data *db = list_entry(b, > struct r5pending_data, sibling); > - if (da->sector > db->sector) > - return 1; > - if (da->sector < db->sector) > - return -1; > - return 0; > + return (da->sector > db->sector) - (da->sector < db->sector); > } > > static void dispatch_defer_bios(struct r5conf *conf, int target,
On Sun, Sep 03, 2023 at 06:30:58PM +0500, Roman Mamedov wrote: > On Sun, 3 Sep 2023 17:50:59 +0800 > Kuan-Wei Chiu <visitorckw@gmail.com> wrote: > > > Replace the conditional statements in the cmp_stripe() function with a > > branchless version to improve code readability and potentially enhance > > performance. > > The new code will always do two comparisons and a subtraction (3 > instructions in total), whereas the old version could return after just 1 > comparison, or after 2 comparisons. So depending on the data values it is 3x > to 1.5x as much operations performed than before, there unlikely to be any > enhancement of performance. > > Also IMO the previous version is more easily readable. > The reason behind my proposed changes was to eliminate conditional branches in the code. While the original code could occasionally achieve early returns, many compilers, such as x86-64 gcc 13.2 compiling with -O2 flag, still generate branch instructions. Processors typically have deep pipelines, and a branch prediction miss can result in a high penalty. Therefore, even though early return might not be possible, the new branchless version of code could still offer efficiency improvements. > > The new code calculates the result using a subtraction of > > comparison results, making it more concise and avoiding conditional > > branches. This change does not alter the functionality of the code. > > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > --- > > drivers/md/raid5.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > > index 4cb9c608ee19..b14d7ba38f0f 100644 > > --- a/drivers/md/raid5.c > > +++ b/drivers/md/raid5.c > > @@ -1035,11 +1035,7 @@ static int cmp_stripe(void *priv, const struct list_head *a, > > struct r5pending_data, sibling); > > const struct r5pending_data *db = list_entry(b, > > struct r5pending_data, sibling); > > - if (da->sector > db->sector) > > - return 1; > > - if (da->sector < db->sector) > > - return -1; > > - return 0; > > + return (da->sector > db->sector) - (da->sector < db->sector); > > } > > > > static void dispatch_defer_bios(struct r5conf *conf, int target, > > > -- > With respect, > Roman -- Best regards, Kuan-Wei Chiu
On Sun, Sep 3, 2023 at 1:10 PM Kuan-Wei Chiu <visitorckw@gmail.com> wrote: > > On Sun, Sep 03, 2023 at 06:30:58PM +0500, Roman Mamedov wrote: > > On Sun, 3 Sep 2023 17:50:59 +0800 > > Kuan-Wei Chiu <visitorckw@gmail.com> wrote: > > > > > Replace the conditional statements in the cmp_stripe() function with a > > > branchless version to improve code readability and potentially enhance > > > performance. > > > > The new code will always do two comparisons and a subtraction (3 > > instructions in total), whereas the old version could return after just 1 > > comparison, or after 2 comparisons. So depending on the data values it is 3x > > to 1.5x as much operations performed than before, there unlikely to be any > > enhancement of performance. > > > > Also IMO the previous version is more easily readable. > > > The reason behind my proposed changes was to eliminate conditional > branches in the code. While the original code could occasionally achieve > early returns, many compilers, such as x86-64 gcc 13.2 compiling with > -O2 flag, still generate branch instructions. Processors typically have > deep pipelines, and a branch prediction miss can result in a high > penalty. Therefore, even though early return might not be possible, the > new branchless version of code could still offer efficiency > improvements. We need more information to support the efficiency improvement here. In this case, I would like to see some benchmark results (micro benchmark is fine). If we cannot show the difference in performance, I would rather keep current code. Thanks, Song [...]
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 4cb9c608ee19..b14d7ba38f0f 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -1035,11 +1035,7 @@ static int cmp_stripe(void *priv, const struct list_head *a, struct r5pending_data, sibling); const struct r5pending_data *db = list_entry(b, struct r5pending_data, sibling); - if (da->sector > db->sector) - return 1; - if (da->sector < db->sector) - return -1; - return 0; + return (da->sector > db->sector) - (da->sector < db->sector); } static void dispatch_defer_bios(struct r5conf *conf, int target,
Replace the conditional statements in the cmp_stripe() function with a branchless version to improve code readability and potentially enhance performance. The new code calculates the result using a subtraction of comparison results, making it more concise and avoiding conditional branches. This change does not alter the functionality of the code. Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> --- drivers/md/raid5.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)