[17/17] vsprintf: rework bitmap_list_string

Commit Message

Yury Norov Aug. 14, 2021, 9:17 p.m. UTC

bitmap_list_string() is very ineffective when printing bitmaps with long
ranges of set bits because it calls find_next_bit for each bit in the
bitmap.  We can do better by detecting ranges of set bits.

In my environment, before/after is 943008/31008 ns.

Signed-off-by: Yury Norov <[e-mail]>
Tested-by: Wolfram Sang <[e-mail]>
---
 lib/vsprintf.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

Comments

Andy Shevchenko Aug. 15, 2021, 11:09 a.m. UTC | #1

On Sun, Aug 15, 2021 at 12:21 AM Yury Norov <[e-mail]> wrote:
>
> bitmap_list_string() is very ineffective when printing bitmaps with long
> ranges of set bits because it calls find_next_bit for each bit in the
> bitmap.  We can do better by detecting ranges of set bits.
>
> In my environment, before/after is 943008/31008 ns.

I would add a couple of words, maybe in parentheses, to describe what
your environment is.

...

> +               buf = number(++buf, end, rtop - 1, default_dec_spec);

++buf is a bit confusing here. Since you will rewrite the buf value
anyway, I would write the parameter as buf + 1.

Yury Norov Aug. 17, 2021, 4:35 p.m. UTC | #2

On Sun, Aug 15, 2021 at 02:09:45PM +0300, Andy Shevchenko wrote:
> On Sun, Aug 15, 2021 at 12:21 AM Yury Norov <[e-mail]> wrote:
> >
> > bitmap_list_string() is very ineffective when printing bitmaps with long
> > ranges of set bits because it calls find_next_bit for each bit in the
> > bitmap.  We can do better by detecting ranges of set bits.
> >
> > In my environment, before/after is 943008/31008 ns.
> 
> I would add a couple of words, maybe in parentheses, to describe what
> your environment is.
> 
> ...
> 
> > +               buf = number(++buf, end, rtop - 1, default_dec_spec);
> 
> ++buf is a bit confusing here. Since you will rewrite the buf value
> anyway, I would write the parameter as buf + 1.

Agree, it's sloppy. I'll  send the patch by tomorrow.

Petr Mladek Aug. 26, 2021, 2:15 p.m. UTC | #3

On Sat 2021-08-14 14:17:13, Yury Norov wrote:
> bitmap_list_string() is very ineffective when printing bitmaps with long
> ranges of set bits because it calls find_next_bit for each bit in the
> bitmap.  We can do better by detecting ranges of set bits.
> 
> In my environment, before/after is 943008/31008 ns.
> 
> Signed-off-by: Yury Norov <[e-mail]>
> Tested-by: Wolfram Sang <[e-mail]>

I like the patch. The new code is much easier to follow.
Feel free to use:

Reviewed-by: Petr Mladek <[e-mail]>

Best Regards,
Petr

Yury Norov Aug. 26, 2021, 8:59 p.m. UTC | #4

On Thu, Aug 26, 2021 at 04:15:09PM +0200, Petr Mladek wrote:
> On Sat 2021-08-14 14:17:13, Yury Norov wrote:
> > bitmap_list_string() is very ineffective when printing bitmaps with long
> > ranges of set bits because it calls find_next_bit for each bit in the
> > bitmap.  We can do better by detecting ranges of set bits.
> > 
> > In my environment, before/after is 943008/31008 ns.
> > 
> > Signed-off-by: Yury Norov <[e-mail]>
> > Tested-by: Wolfram Sang <[e-mail]>
> 
> I like the patch. The new code is much easier to follow.
> Feel free to use:
> 
> Reviewed-by: Petr Mladek <[e-mail]>
> 
> Best Regards,
> Petr

Thanks Petr! The patch is already in the linux-next.

Andrew, Stephen, can you please append Petr's reviewed-by?

Thanks,
Yury

Patch

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd006adfe853..29a384eee286 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1241,20 +1241,13 @@  char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 			 struct printf_spec spec, const char *fmt)
 {
 	int nr_bits = max_t(int, spec.field_width, 0);
-	/* current bit is 'cur', most recently seen range is [rbot, rtop] */
-	int cur, rbot, rtop;
 	bool first = true;
+	int rbot, rtop;
 
 	if (check_pointer(&buf, end, bitmap, spec))
 		return buf;
 
-	rbot = cur = find_first_bit(bitmap, nr_bits);
-	while (cur < nr_bits) {
-		rtop = cur;
-		cur = find_next_bit(bitmap, nr_bits, cur + 1);
-		if (cur < nr_bits && cur <= rtop + 1)
-			continue;
-
+	for_each_set_bitrange(rbot, rtop, bitmap, nr_bits) {
 		if (!first) {
 			if (buf < end)
 				*buf = ',';
@@ -1263,15 +1256,12 @@  char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 		first = false;
 
 		buf = number(buf, end, rbot, default_dec_spec);
-		if (rbot < rtop) {
-			if (buf < end)
-				*buf = '-';
-			buf++;
-
-			buf = number(buf, end, rtop, default_dec_spec);
-		}
+		if (rtop == rbot + 1)
+			continue;
 
-		rbot = cur;
+		if (buf < end)
+			*buf = '-';
+		buf = number(++buf, end, rtop - 1, default_dec_spec);
 	}
 	return buf;
 }