Re: bmake inefficiencies

[Mateusz Guzik <mjguzik%gmail.com@localhost>]

Responding to the original mail due to several follow ups.

Thanks for fixups so far.

Another note related to JobOpenTmpFile:

        JobSigLock(&mask);
        tfd = mkTempFile(TMPPAT, &tfile);
        if (!DEBUG(SCRIPT))
                (void)eunlink(tfile);
        JobSigUnlock(&mask);

is the signal stuff really necessary?

Apart from that, eunlink stats the path and this has happens in
default setting. Instead I propose the following: since mkTempFile has
only 2 consumers, passing in the buffer can be made mandatory. Then
another flag can control when the routine should unlink the result.
This would shorten this callsite to roughly:

char buf[MAXPATHLATEN];
...
tfd = mkTempFile(TMPPAT, buf, sizeof buf, !DEBUG(SCRIPT));

which will also save on memory alloc and string coying as the target
routine calls bmake_strdup to accomodate the request.

On 1/26/21, Mateusz Guzik <mjguzik%gmail.com@localhost> wrote:
> I had a look with truss at what bmake is doing and I think a lot of
> the syscalls can be easily avoided. Examples come from FreeBSD with
> bmake-20210110 and I don't know to what extent they are representative
> for other systems.
>
> 1. spurious waitpid
>
> when doing buildkernel I counted the following:
> 7010 cases of error code 0 (== nothing to do)
> 371 cases of ECHILD
> 1526 cases of reaping
>
> To my understanding bmake knows when there are any children to reap
> and at least the first case can be easily avoided.
>
> 2. FD_CLOEXEC
>
> for example mkstemp is used and FD_CLOEXEC performed on the result.
> Instead mkostemp can be used and mkTempFile can accept O_CLOEXEC as an
> argument to save explicit fcntl.
>
> 3. avoidable temp files
>
> JobOpenTmpFile opens a file and later checks if anything was written
> to it. Not an actual patch, but a hack to demonstrate the fix can be
> found at the end. Looks like checking can be done upfront in O(1) time
> (not done in the patch below).
>
> With this in place I get the following result for buildworld:
> 87930 JobOpenTmpFile: created!
> 70564 JobOpenTmpFile: skipped!
>
> i.e. about 44% of all temp files can be avoided at least in this workload.
>
> There is probably more, but I did not want to analyze further.
>
> Is this something you can address?
>
> Thanks,
>
> diff --git a/contrib/bmake/job.c b/contrib/bmake/job.c
> index d43761ca80ff..67b75f228547 100644
> --- a/contrib/bmake/job.c
> +++ b/contrib/bmake/job.c
> @@ -1022,6 +1022,24 @@ JobPrintCommand(Job *job, ShellWriter *wr,
> StringListNode *ln, const char *ucmd)
>   *
>   * Return whether at least one command was written to the shell file.
>   */
> +static Boolean
> +JobWillPrintCommands(Job *job)
> +{
> +       StringListNode *ln;
> +       Boolean seen = FALSE;
> +
> +       for (ln = job->node->commands.first; ln != NULL; ln = ln->next) {
> +               const char *cmd = ln->datum;
> +
> +               if (strcmp(cmd, "...") == 0) {
> +                       break;
> +               }
> +               seen = TRUE;
> +       }
> +
> +       return seen;
> +}
> +
>  static Boolean
>  JobPrintCommands(Job *job)
>  {
> @@ -1609,6 +1627,15 @@ JobOpenTmpFile(Job *job, GNode *gn, Boolean
> cmdsOK, Boolean *out_run)
>                 DieHorribly();
>         }
>
> +       if (getenv("MAKE_TRYSKIP") != NULL) {
> +               if (!JobWillPrintCommands(job)) {
> +                       printf("%s: skipped!\n", __func__);
> +                       *out_run = FALSE;
> +                       return;
> +               }
> +               printf("%s: created!\n", __func__);
> +       }
> +
>         JobSigLock(&mask);
>         tfd = mkTempFile(TMPPAT, &tfile);
>         if (!DEBUG(SCRIPT))
>
>
> -
>
> --
> Mateusz Guzik <mjguzik gmail.com>
>


-- 
Mateusz Guzik <mjguzik gmail.com>