Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> The choice from using the max 80 char width comes from a time where we > did not usually add the bloat-o-meter output to commit messages, but i > agree that we should shorten it to silence the patch checkers. I like > Dietmars suggestion to keep the numbers aligned. > > What is missing in our variant is support for -p "ARCH-PREFIX-" for > readelf. Should probably switch to argparse too. argparse and prefix sounds cool! Thanks Bernhard, i will add this to my todo list. -- Regards, Jones Syue | 薛懷宗 QNAP Systems, Inc. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
On Mon, 11 Mar 2024 18:39:33 +0800 Kang-Che Sung wrote: > I was curious. Is there a reason for BusyBox's bloat-o-meter script not to > keep in sync with the version that comes in the Linux kernel source? > > I occasionally use the bloat-o-meter from the Linux kernel to compare even > BusyBox binaries. There shouldn't be any functional differences between the > two versions. IIRC the variant living in the kernel does not handle symbol aliases properly and uses nm(1). Our variant uses readelf(1) and works also for code that uses aliases, like your favourite libc and other such binaries. The choice from using the max 80 char width comes from a time where we did not usually add the bloat-o-meter output to commit messages, but i agree that we should shorten it to silence the patch checkers. I like Dietmars suggestion to keep the numbers aligned. What is missing in our variant is support for -p "ARCH-PREFIX-" for readelf. Should probably switch to argparse too. HTH ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> He didn't mean to reduce the length of the line to 72, but to align > the number (328 bytes) in the example with the last column of previous > lines. Is one thing I also noted that your patch changed. Note the > final 'bytes' word changed side. Ohh okay now i see! Thank you Xabier and Dietmar for kindly feedback :) the 'bytes' is switched from the right hand side to the left hand side of the numbers '328'. Will come out next v3 later :) <... cut ...> 72 crond_dummy_global_int - 4 +4 75 --- 75 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total: 328 bytes 72 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total bytes: 328 -- Regards, Jones Syue | 薛懷宗 QNAP Systems, Inc. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
Hi Jones, > > How about > > > > 72 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total bytes: > > 328 > > > > to keep the numbers aligned? > > Yes! 72 is totally aligned with function/old/new/delta length! :) > And IMHO 75 columns is still a bit preferred than 72 columns, two reasons > are considered: He didn't mean to reduce the length of the line to 72, but to align the number (328 bytes) in the example with the last column of previous lines. Is one thing I also noted that your patch changed. Note the final 'bytes' word changed side. Cheers, Xabier Oneca_,,_ ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> How about
>
> 72 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total bytes: 328
>
> to keep the numbers aligned?
Yes! 72 is totally aligned with function/old/new/delta length! :)
And IMHO 75 columns is still a bit preferred than 72 columns, two reasons
are considered:
1. consistent with linux kernel upstream.
Found this thread had a v1 proposal patch to do line wrapped at 72 columns:
https://lore.kernel.org/lkml/1427827225.18175.3.ca...@perches.com/T/#u
and finally 75 columns goes to upstream, Commit 2a076f40d8c9b ("checkpatch,
SubmittingPatches: suggest line wrapping commit messages at 75 columns"):
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=2a076f40d8c9b
2. easy to maintain and grep keyword '75'.
75-columns patch uses 's/78/75; s/80/77/' replacement and keeps keyword
'75' in the file, if someday wide screen is very popular and output width
exceeded that said 100 columns would not ruin people's life, we could
easily find the expected length '75' and replace it with a wider numbers.
72-columns patch uses 's/78/72; s/80/74/' replacement might a bit hard to
find the expected length afterward.
By the way, loop is not required in my previous length counting script :)
simply pipe to awk is fine:
~/busybox/scripts/bloat-o-meter ~/busybox_unstripped_{orig,fix} \
| awk '{ print length, $0 }'
--
Regards,
Jones Syue | 薛懷宗
QNAP Systems, Inc.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
RE: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> From: busybox On Behalf Of Jones Syue ???
> Sent: Wednesday, March 13, 2024 2:22 AM
>
> > This patch replaces the 78 "-" prints with 75 "-". And replace the 80
> > columns summary line with 77 columns. ("%s" is considered as two chars
> > and should be filled with whitespace " ", so 77 = 75 + 2)
>
> This is an example which showing how this patch wrapped line to 75 columns,
> use awk to count the line length and prepend it to each line's head[1].
> Before applying this patch, the seperation line and summary line are
> wrapped to 78 columns:
>
> 72 crond_dummy_func - 211+211
> 72 .rodata59300 59404+104
> 72 crond_main 704 709 +5
> 72 static.crond_dummy_local_int - 4 +4
> 72 crond_dummy_global_int - 4 +4
> 78
> --
> 78 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total: 328
> bytes
>
> After applying this patch, the seperation line and summary line are
> wrapped to 75 columns:
>
> 72 crond_dummy_func - 211+211
> 72 .rodata59300 59404+104
> 72 crond_main 704 709 +5
> 72 static.crond_dummy_local_int - 4 +4
> 72 crond_dummy_global_int - 4 +4
> 75 ---
> 75 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total: 328 bytes
How about
72 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total bytes: 328
to keep the numbers aligned?
--
Best regards,
Dietmar Schindler
manroland Goss web systems GmbH | Managing Director: Franz Kriechbaum
Registered Office: Augsburg | Trade Register: AG Augsburg | HRB-No.: 32609 |
VAT: DE815764857
Confidentiality note:
This message and any attached documents may contain confidential or proprietary
information of manroland|Goss. These materials are intended only for the use of
the intended recipient. If you are not the intended recipient of this
transmission, you are hereby notified that any distribution, disclosure,
printing, copying, storage, modification or the taking of any action in
reliance upon this transmission is strictly prohibited. Delivery of this
message to any person other than the intended recipient shall not compromise or
waive such confidentiality, privilege or exemption from disclosure as to this
communication. If you have received this communication in error, please
immediately notify the sender and delete the message from your system. All
liability for viruses is excluded to the fullest extent permitted by law.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> This patch replaces the 78 "-" prints with 75 "-". And replace the 80
> columns summary line with 77 columns. ("%s" is considered as two chars
> and should be filled with whitespace " ", so 77 = 75 + 2)
This is an example which showing how this patch wrapped line to 75 columns,
use awk to count the line length and prepend it to each line's head[1].
Before applying this patch, the seperation line and summary line are
wrapped to 78 columns:
72 crond_dummy_func - 211+211
72 .rodata59300 59404+104
72 crond_main 704 709 +5
72 static.crond_dummy_local_int - 4 +4
72 crond_dummy_global_int - 4 +4
78
--
78 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total: 328
bytes
After applying this patch, the seperation line and summary line are
wrapped to 75 columns:
72 crond_dummy_func - 211+211
72 .rodata59300 59404+104
72 crond_main 704 709 +5
72 static.crond_dummy_local_int - 4 +4
72 crond_dummy_global_int - 4 +4
75 ---
75 (add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total: 328 bytes
[1]
while read -r line; \
do \
awk '{ print length, $line }'; \
done < <(~/busybox/scripts/bloat-o-meter ~/busybox_unstripped_{orig,fix});
--
Regards,
Jones Syue | 薛懷宗
QNAP Systems, Inc.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> Per test it looks like busybox has one more items: ".rodata", if the
> modification contains a format strings like this:
> log5("crond_dummy 1 %d %d\n", crond_dummy_global_int, crond_dummy_local_int);
Looks like the formatted strings is considered as a string literal, not a
symbol, while linux kernel's bloat-o-meter is doing symbol lookup through
'nm', which could not find the diff of string literal, and show nothing
change in the 'RO Data' category. The string literal is resident in the
.rodata section, and busybox's bloat-o-meter is reading the .rodata section
through 'readelf' so it could find the size diff.
--
Regards,
Jones Syue | 薛懷宗
QNAP Systems, Inc.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
> I was curious. Is there a reason for BusyBox's bloat-o-meter script not to
> keep in sync with the version that comes in the Linux kernel source?
It looks like busybox's bloat-o-meter has this addon: .rodata section diff.
Commit f14f7fc5cad5 ("Teach bloatometer about .rodata, and tweak the
display into something that") introduced this feature through 'readelf'.
https://git.busybox.net/busybox/commit/?id=f14f7fc5cad5
After poking around mailing list found this thread has a talk about
"[PATCH] update kernel's scripts/bloat-o-meter from busybox", and somehow
not goes to linux kernel upstream:
https://lore.kernel.org/all/200906020104.56471@landley.net/t/
linux kernel's bloat-o-meter has an alternative checks for read-only
sections. Commit c50e3f512a5a ("bloat-o-meter: include read-only data
section in report) introduced it through 'nm' with type 'r' or 'R'.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=c50e3f512a5a
> I occasionally use the bloat-o-meter from the Linux kernel to compare even
> BusyBox binaries. There shouldn't be any functional differences between the
> two versions.
Per test it looks like busybox has one more items: ".rodata", if the
modification contains a format strings like this:
log5("crond_dummy 1 %d %d\n", crond_dummy_global_int, crond_dummy_local_int);
The output of two versions:
~/busybox/scripts/bloat-o-meter ~/busybox_unstripped_{orig,fix}
function old new delta
crond_dummy_func - 211+211
.rodata59300 59404+104
crond_main 704 709 +5
static.crond_dummy_local_int - 4 +4
crond_dummy_global_int - 4 +4
--
(add/remove: 3/0 grow/shrink: 2/0 up/down: 328/0) Total: 328 bytes
~/git/linux/scripts/bloat-o-meter -c ~/busybox_unstripped_{orig,fix}
add/remove: 1/0 grow/shrink: 1/0 up/down: 216/0 (216)
Function old new delta
crond_dummy_func - 211+211
crond_main 704 709 +5
Total: Before=692144, After=692360, chg +0.03%
add/remove: 2/0 grow/shrink: 0/0 up/down: 8/0 (8)
Data old new delta
crond_dummy_local_int - 4 +4
crond_dummy_global_int - 4 +4
Total: Before=13500, After=13508, chg +0.06%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
RO Data old new delta
Total: Before=35929, After=35929, chg +0.00%
--
Regards,
Jones Syue | 薛懷宗
QNAP Systems, Inc.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
Jones Syue 薛懷宗 於 2024年3月11日 星期一寫道：
> This patch replaces the 78 "-" prints with 75 "-". And replace the 80
> columns summary line with 77 columns. ("%s" is considered as two chars
> and should be filled with whitespace " ", so 77 = 75 + 2)
>
> Consider this scenario: a patch contains the output of "bloat-o-meter" to
> clarify about the size impact/diff, and when we validate this patch with
> "~/linux/scripts/checkpatch.pl" (from linux kernel source tree), which
> checks for style violations, it might complain about line wrapped like:[1]
> WARNING: \
> Possible unwrapped commit description (prefer a maximum 75 chars per line)
>
> The 1st complaint is seperation line with 78 '-' prints, and the 2nd
> complaint is summary line "(add/remove ... Total: n bytes)". Although
> these two warnings are not harmful at all, it is helpful and makes life
> easier if this kind of patch (with "bloat-o-meter" output) could be passed
> by 'checkpatch.pl' in the first place without manually inspection.
>
> [1] line wrapped at 75 columns:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> Signed-off-by: Jones Syue
> ---
> v2:
> - fix nit/typo with correct word 'scenario'
> v1: http://lists.busybox.net/pipermail/busybox/2024-March/090656.html
> ---
> scripts/bloat-o-meter | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
I was curious. Is there a reason for BusyBox's bloat-o-meter script not to
keep in sync with the version that comes in the Linux kernel source?
I occasionally use the bloat-o-meter from the Linux kernel to compare even
BusyBox binaries. There shouldn't be any functional differences between the
two versions.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2] bloat-o-meter: line wrapped at 75 columns instead of 80
This patch replaces the 78 "-" prints with 75 "-". And replace the 80
columns summary line with 77 columns. ("%s" is considered as two chars
and should be filled with whitespace " ", so 77 = 75 + 2)
Consider this scenario: a patch contains the output of "bloat-o-meter" to
clarify about the size impact/diff, and when we validate this patch with
"~/linux/scripts/checkpatch.pl" (from linux kernel source tree), which
checks for style violations, it might complain about line wrapped like:[1]
WARNING: \
Possible unwrapped commit description (prefer a maximum 75 chars per line)
The 1st complaint is seperation line with 78 '-' prints, and the 2nd
complaint is summary line "(add/remove ... Total: n bytes)". Although
these two warnings are not harmful at all, it is helpful and makes life
easier if this kind of patch (with "bloat-o-meter" output) could be passed
by 'checkpatch.pl' in the first place without manually inspection.
[1] line wrapped at 75 columns:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
Signed-off-by: Jones Syue
---
v2:
- fix nit/typo with correct word 'scenario'
v1: http://lists.busybox.net/pipermail/busybox/2024-March/090656.html
---
scripts/bloat-o-meter | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/bloat-o-meter b/scripts/bloat-o-meter
index b4a1d28113b0..7fe4a06ae19e 100755
--- a/scripts/bloat-o-meter
+++ b/scripts/bloat-o-meter
@@ -130,10 +130,10 @@ for d, n in delta:
old_sz = old.get(n, {}).get("size", "-")
new_sz = new.get(n, {}).get("size", "-")
print("%-48s %7s %7s %+7d" % (n, old_sz, new_sz, d))
-print("-"*78)
+print("-"*75)
total="(add/remove: %s/%s grow/shrink: %s/%s up/down: %s/%s)%%sTotal: %s
bytes"\
% (add, remove, grow, shrink, up, -down, up-down)
-print(total % (" "*(80-len(total
+print(total % (" "*(77-len(total
if flag_timing:
print("\n%d/%d; %d Parse origin/new; processing nsecs" %
(end_t1-start_t1, end_t2-start_t2, end_t3-start_t3))
--
2.1.4
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
