Re: [PATCH] eliminate some installation overhead

[Ralf Wildenhues]

Hi Jim,

* Jim Meyering wrote on Mon, May 26, 2008 at 05:05:01PM CEST:
> 
> Since part of coreutils' install program (currently ifdef'd out) incurs
> a performance hit due to an SELinux implementation problem (I hear that
> a solution is in the works), I'm motivated to continue Ralf's work of
> minimizing automake's practice of exec'ing install once per installed
> file.

You just barely beat me!  :-)

> This first installment addresses man pages.
> I've tested it with coreutils, and as expected, it results in a single
> invocation of install for all man/*.1 files, rather than 100 separate
> invocations, where they were being done one at a time.

This assumes that there is no command line length limit, this could be
an issue here.  The command line must anyway cope with one instance of
`%..._LIST%'.  However, for the install command, your code may enlarge
that by `$(srcdir)/' for each file.  As an example package, OpenMPI
has roughly 300 man3 manpages, which with a relative source dir of
../$PACKAGE enlarges the install command line from roughly 9K to 17K.
Libtool's LT_CMD_MAX_LEN macro lists 8/12/16K for some systems, it's not
clear to me how much safety zone was included there.

> This is mainly a heads-up, to see if there are any objections.
> If not, I'll write a NEWS entry and a test or two.

A NEWS entry and a couple of tests would be great, but fixing the above
is needed, too, as well as the nits below.  Note that I'm quite OK with
reworking this patch myself, if you don't have enough time; but I surely
would welcome if your motivation keeps you going.  ;-)

> --- a/lib/am/mans.am
> +++ b/lib/am/mans.am
> @@ -72,6 +72,25 @@ if %?TRANS_MANS%
>  ?HAVE_TRANS?     *.%SECTION%*) list="$$list $$i" ;; \
>  ?HAVE_TRANS?   esac; \
>  ?HAVE_TRANS? done; \
> +     install_all_at_once=1; \

What about  s/install_all_at_once=1/install_multiple=:/
and setting that to false if needed, adjusting everywhere?
That way I don't have to nit about using `=' vs. `-eq' below.

> +     case '$(program_transform_name)' in \
> +       s,x,x,) for i in $$list; do \

I've seen s,y,y, being used as well for the trivial case, though
  git log -p -Sprogram_transform_name

shows that that hasn't been in Autoconf's history; also, there
is code that specifically checks for "s,x,x,".

On anoter note, you're suboptimal here in that you turn off installation
all at once for notrans_ manpages as well, which is the new feature to
avoid manpage name transformation with --program-* at all.  :-)

> +         case $$i in *.%SECTION%) ;; *) install_all_at_once=0;; esac;done;;\

Please put a space after `esac;'.

> +       *) install_all_at_once=0 ;; esac; \

Please no space before `;;' (only for consistency with the line above).

> +     if test $$install_all_at_once = 1; then \
> +## convert names in $$list to be $(srcdir)-relative, if needed
> +       new_list=; \
> +       for i in $$list; do \
> +## Find the file.
> +         if test -f $$i; then file=$$i; \
> +         else file=$(srcdir)/$$i; fi; \
> +         new_list="$$new_list $$file"; \
> +       done; \
> +       list=$$new_list; \
> +       echo " $(INSTALL_DATA) $$list $(DESTDIR)$(man%SECTION%dir)"; \

Please put single quotes around $(DESTDIR)$(man%SECTION%dir) in the echo
line.

> +       $(INSTALL_DATA) $$list "$(DESTDIR)$(man%SECTION%dir)"; \
> +       exit 0; \

Hmm, `exit $?' would seem more appropriate here, although it would fix
but one of the cases.  Maybe better to fix them all in a separate patch
(including test exposure for each code path).

> +     fi; \
>       for i in $$list; do \
>  ## Find the file.
>         if test -f $$i; then file=$$i; \

Cheers, and thanks for the patch!
Ralf