diff mbox series

[17/17] vsprintf: rework bitmap_list_string

Message ID 20210814211713.180533-18-yury.norov@gmail.com (mailing list archive)
State New, archived
Headers show
Series Resend bitmap patches | expand

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 <yury.norov@gmail.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 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 <yury.norov@gmail.com> 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 <yury.norov@gmail.com> 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 <yury.norov@gmail.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

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

Reviewed-by: Petr Mladek <pmladek@suse.com>

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 <yury.norov@gmail.com>
> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I like the patch. The new code is much easier to follow.
> Feel free to use:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> 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
diff mbox series

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;
 }