Re: [PATCH 1/1] boot: Enhance performance by eliminating unnecessary calls to printf()

[Bryan Donlan <bdonlan@gmail.com>]

On Sun, Jun 12, 2011 at 14:36, Jean Sacren <sakiwit@gmail.com> wrote:
> From: "H. Peter Anvin" <hpa@linux.intel.com>
> Date: Mon, 06 Jun 2011 10:08:32 -0700
>>
>> On 06/05/2011 05:40 PM, Jean Sacren wrote:

>> First of all, this is a build time tool which is executed exactly once
>> during the entire kernel build.
>>
>> Second, printf execution time is largely dependent on the size
>> formatting string; since the I/O is buffered it is only issued once
>> anyway... which basically means that there is no time saved at all.
>
> The above two arguments have nothing to do with the fact you printed out
> 13 lines individually, where they should have been printed out
> collectively. To make my point here, why you didn't print out each
> character individually?

Because that would be substantially harder to read. And harder to
write, at that.

>
> I looked at the history of the file and the way you did it is a birth
> shortcoming. For the past two years, no code has ever been necessarily
> inserted between these 13 printf() calls. Looking into the future, that
> block of code shall be facilitated by the updated patch.
>>
>> Third, the resulting code is substantially harder to read.
>
> With the updated patch, this argument doesn't stand at all.

Sure it does. With the original code, the arguments to printf are
right next to the format specifiers; with your patch you have to count
format specifiers to figure out which argument goes to what. And for
what? A millisecond or two of time saved, at compile time, probably
less?

>> Fourth, carrying this as a patch will cost kernel developers more time
>> in additional git execution time than it ever will save them in build time.
>
> With the updated patch, this argument doesn't make any sense at all.

You've already cost kernel developers more time in responding to your
patch than they'll save in build time. Asking them to apply it to git
wastes even more time.

> +       printf(".section \".rodata..compressed\",\"a\",@progbits\n"
> +              ".globl z_input_len\n"
> +              "z_input_len = %lu\n"
> +              ".globl z_output_len\n"
> +              "z_output_len = %lu\n"
> +              ".globl z_extract_offset\n"
> +              "z_extract_offset = 0x%lx\n"
> +              ".globl z_extract_offset_negative\n"
> +              "z_extract_offset_negative = -0x%lx\n"
> +              ".globl input_data, input_data_end\n"
> +              "input_data:\n"
> +              ".incbin \"%s\"\n"
> +              "input_data_end:\n",
> +              ilen, (unsigned long)olen, offs, offs, argv[1]);

Quick! Which printf argument does z_extract_offset correspond to?
If you had to count (or it took you more than half a second
otherwise), this code is less readable than it was before.