Re: [PATCH 3/4] maint.mk: speed up require_config_h_first

[Eric Blake]

On 07/26/2016 08:28 AM, Ján Tomko wrote:
> Instead of spawning three processes per file, rewrite the check
> in perl and run it once for all the files.
> ---
> Alternatively, grep --max-count 1 could help with getting rid of
> the per-file commands and extra sed, but I don't know if it's
> portable enough.

Nope. That's a GNU grep extension, and we know that some developers
still use BSD grep.


> +# Like grep -m 1, this only looks at the first match.
> +perl_config_h_first_ =                                                       
> \
> +    -e 'BEGIN {$$ret = 0}'                                           \
> +    -e 'if (/^\# *include\b/) {'                                     \
> +    -e '  if (not m{^\# *include $(config_h_header)}) {'             \
> +    -e '    print "$$ARGV\n";'                                               
> \
> +    -e '    $$ret = 1;'                                                      
> \
> +    -e '  }'                                                         \
> +    -e '  \# Move on to next file after first include'                       
> \
> +    -e '  close ARGV;'                                                       
> \
> +    -e '}'                                                           \
> +    -e 'END {exit $$ret}'

Looks pretty slick to me. Again, I'd like a second opinion on whether
this is the best use of perl, if anyone else wants to chime in, but will
probably apply these patches late next week if no conversation ensues in
the meantime.

> +
>  # You must include <config.h> before including any other header file.
>  # This can possibly be via a package-specific header, if given by cfg.mk.
>  sc_require_config_h_first:
>       @if $(VC_LIST_EXCEPT) | grep '\.c$$' > /dev/null; then          \
> -       fail=0;                                                       \
> -       for i in $$($(VC_LIST_EXCEPT) | grep '\.c$$'); do             \
> -         grep '^# *include\>' $$i | $(SED) 1q                        \

Even if we don't go with perl, a very obvious speedup would be
converting grep|sed into a single sed, cutting from 3 to 2 forks per
shell loop iteration (untested):

$(SED) -n '/^# *include>/ { p; q; }' < $$i

except that portability of one-line {} in sed is a little bit suspect,
and multiline sed is harder to write in makefiles.

> -             | grep -E '^# *include $(config_h_header)' > /dev/null  \

Less obvious, it may be possible to fold the above sed|grep -E into a
single sed by putting even more code inside the {}, although that is
trickier to write so I'm not attempting it here.

But I still think that one perl process for all files is hands down
better than a more efficient shell loop that has to fork per file,
especially if the cost of forking is the dominant slowdown here (most of
these files are already hot in cache during overall 'make syntax-check',
so I/O is less likely to be the bottleneck). Or put another way, O(1)
always beats O(n), even if I can speed up O(n) by reducing the factor
from 3 to 2 or 1.


> -           || { echo $$i; fail=1; };                                 \
> -       done;                                                         \
> -       test $$fail = 1 &&                                            \
> +       files=$$($(VC_LIST_EXCEPT) | grep '\.c$$') &&                 \
> +       perl -n $(perl_config_h_first_) $$files ||                    \
>           { echo '$(ME): the above files include some other header'   \
>               'before <config.h>' 1>&2; exit 1; } || :;               \
>       else :;                                                         \
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature