lkml.org 
[lkml]   [2011]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/1] boot: Enhance performance by eliminating unnecessary calls to printf()
    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-06-14 21:01    [W:2.710 / U:0.952 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site