Re: [GOLD] add new method for computing a build ID (take 2)

[Geoff Pike <gpike at chromium dot org>]

I think it's in good shape now.

On Mon, Apr 8, 2013 at 1:40 PM, Cary Coutant <ccoutant@google.com> wrote:
>
> > Re: thread-count-final: Yes, I think it is OK to use that.
> >
> > Re: the long option names: I left them as they were, but don't feel strongly.
>
> OK.
>
> > Re: the number of tasks vs the number of threads: I decided to let the
> > mathematical function to compute be independent of the number of
> > threads.  That way (all else being equal) Alice running with
> > --thread-count=8 will get the same build ID as Bob running with
> > --thread-count=4 and Charlie who configured gold with
> > --disable-threads. I slightly rearranged the logic for my tests (in
> > Makefile.am) to reflect this.
>
> Good point. That makes more sense than what I suggested.
>
> A couple of nits that I missed last time:
>
> +
> +
> +// BUILD_ID_BLOCKER_ blocks this.
> +Task_token*
> +Hash_task::is_runnable()
>
> Extra blank line above the comment; need a blank line after the
> comment. The comment itself isn't really all that useful, though --
> it's only stating the obvious.

Removed the comment and the extra blank line.

> +  if (this->build_id_note_ != NULL
> +      && (strcmp(parameters->options().build_id(), "tree") == 0)
> +      && (parameters->options().build_id_chunk_size_for_treehash() > 0)
> +      && (filesize > 0)
> +      && (filesize >=
> +          parameters->options().build_id_min_file_size_for_treehash()))
>
> In the middle three terms, the outer set of parens is unnecessary.
> (The parens are necessary for the last term because of the line
> break.) I wouldn't normally complain about extra parens, but in this
> case I found them a bit distracting. If you're going for parallel
> structure, the first term should have them as well, but I wouldn't go
> in that direction.

Fixed.

> +  // If a treehash is necessary to compute the build ID, then queue
> +  // the necessary tasks and return a blocker that will unblock when
> +  // they finish.  Otherwise return the 2nd arg.
> +  Task_token*
> +  queue_build_id_tasks(Workqueue* workqueue, Task_token* build_id_blocker,
> +                       Output_file* of);
>
> Replace "2nd arg" with "BUILD_ID_BLOCKER".

Fixed.

> +  DEFINE_uint64(build_id_chunk_size_for_treehash,
> +                options::TWO_DASHES, '\0', 2 << 20,
> +                N_("Chunk size for '--build-id=tree'"), N_("SIZE"));
> +
> +  DEFINE_uint64(build_id_min_file_size_for_treehash, options::TWO_DASHES,
> +                '\0', 40 << 20,
> +                N_("Minimum output file size for '--build-id=tree' to work"
> +                   " differently than '--build-id=sha1'"), N_("SIZE"));
>
> Not even a nit, just an idea for a future patch: I'd like to see an
> option syntax that allows "2M" for "2097152" or "0x200000"
> (DEFINE_uint64_scaled?). There may be a few other options that could
> use that, too.
>
> Looks good to me. Thanks!
>
> -cary

Thanks for the review!

Geoff

        * gold.cc (queue_final_tasks): invoke layout->queue_build_id_tasks().
        * layout.cc (Hash_task): New class.
        (Layout::queue_build_id_tasks): New function.
        (Layout::write_build_id): Handle single-thread portion of build ID
        computation.  (In some cases, all of it is single-threaded.)  Replace
        {sha1,md5}_process_bytes with {sha1,md5}_buffer to get the same
        functionality in fewer lines of code.
        * layout.h (Layout::queue_build_id_tasks): New function declaration.
        * options.h (General_options): make "--build-id" default to tree
        rather than sha1.  Add two new options related to --build-id=tree:
        --build-id-chunk-size-for-treehash and
        --build-id-min-file-size-for-treehash.
        * Makefile.am: add testing of --build-id=tree and related new options
        (these tests will be invoked by "make check")
        * Makefile.in: regenerate

Attachment: t.patch
Description: Binary data