makefiles: use 'install' instead of 'cp' in 'make install' targets by gasche · Pull Request #1680 · ocaml/ocaml

gasche · 2018-03-28T18:12:09Z

I can observe weird performance bottlenecks on my machine caused by
the use of 'cp' in the 'install' scripts of OCaml. When installing
into a directory that is already populated by an existing
installation, 'make install' can routinely take 10s on my machine¹. After this
change it reliably takes 1.5s, independently of whether the
destination is already populated or not.

¹: a brtfs filesystem on an old-ish SSD

Why I care

An extra 10s delay due to 'make install' can be noticeable in tight
change-build-install-test feedback loops for a compiler change where
we change the compiler, have a fast 'make world.opt' due to
incremental builds, install the change and test it -- possibly after
installing a couple opam packages, which can be fairly quick.

Partial diagnosis

The performance issue seems to be caused by the fact that 'cp' (at
least the GNU coreutils version), when the file already exists,
replaces it by opening it in writeonly+truncate mode and writing the
file content ('strace' shows that the delay is caused within an
'openat' call). In particular, using the --remove-destination option
(which changes 'cp' to just remove the destination file before
copying) removes the performance issue, but this option seems missing
from the BSD/OSX 'cp' so it could cause portability issue.

Change

The present commit rewrites the 'install' targets of all Makefiles to
use the 'install' command instead. 'install' by default gives
executable-like permission to the destination file, instead of reusing
the source file's permissions, so we specify manually the permission
modes, depending on whether the installed file is an executable (or
dynamically-linked library) or just data (including other compiled
object files).

Testing

I checked manually that the permissions of the installed files are
identical to the ones of the current 'cp'-using targets, except for
some '.mli' file in middle_end which currently have +x bits enabled
for no good reason.

Remark: To test this, playing with the DESTDIR variable is very useful
(this lets you install to a new directory (or the same as before)
without having to re-run the configure script). I used the following,
fairly slow shell script to collect permissions:

for f in $(find $DESTDIR); do \
  echo $(basename $f) $(ls -l $f | cut -d' ' -f1); \
done | sort

Remark: it is important to run sync in-between 'make install' runs
to avoid timing effects due to filesystem or disk caching
strategies. I believe that this corresponds to the natural time delay
(and unrelated disk activity) that would occur in realistic
change-install-test feedback loops.

shindere · 2018-03-29T10:06:33Z

In the root Makefile, I'd define the variables before their first use, I'd find this cleaner. In all the files: I'd rather use numerical, "abbsolute permissions" than permissions with letters. this also has the advantage that the permissions of installed files do not depend on those of the files in the repository, which happen to be wrong, sometimes. Also, I'd store "install" in a varialbe to avoid hardcoding it in too many places, to make it possible / easier to specify a path to the program. So basically, here is how I owuld proceed: INSTALL := install INSTALL_DATA := $(INSTALL) -m 664 INSTALL_PROGRAMS := $(INSTALL) -m 755 and then use the two latter in the recipes. Also note that this PR will close a mantis ticket that was requesting exactly this. I don't have the number with me right now but I think you should be able to find it easily.

gasche · 2018-03-29T12:37:18Z

Thanks! The Mantis ticket is MPR#5219.

I performed the changes you suggested¹, and also created a shared Makefile.common file to factorize all the INSTALL_* definitions duplicated in all the makefiles.

¹: except for one, I kept the letters instead of using octal modes, because I don't find the octal modes very usable, I never remember which action is which bit.

I can observe weird performance bottlenecks on my machine caused by the use of 'cp' in the 'install' scripts of OCaml. When installing into a directory that is already populated by an existing installation, 'make install' can routinely take 10s on my machine¹. After this change it reliably takes 1.5s, independently of whether the destination is already populated or not. ¹: a brtfs filesystem on an old-ish SSD Why I care ---------- An extra 10s delay due to 'make install' can be noticeable in tight change-build-install-test feedback loops for a compiler change where we change the compiler, have a fast 'make world.opt' due to incremental builds, install the change and test it -- possibly after installing a couple opam packages, which can be fairly quick. Partial diagnosis ----------------- The performance issue seems to be caused by the fact that 'cp' (at least the GNU coreutils version), when the file already exists, replaces it by opening it in writeonly+truncate mode and writing the file content ('strace' shows that the delay is caused within an 'openat' call). In particular, using the --remove-destination option (which changes 'cp' to just remove the destination file before copying) removes the performance issue, but this option seems missing from the BSD/OSX 'cp' so it could cause portability issue. Change ------ The present commit rewrites the 'install' targets of all Makefiles to use the 'install' command instead. 'install' by default gives executable-like permission to the destination file, instead of reusing the source file's permissions, so we specify manually the permission modes, depending on whether the installed file is an executable (or dynamically-linked library) or just data (including other compiled object files). Testing ------- I checked manually that the permissions of the installed files are identical to the ones of the current 'cp'-using targets, except for some '.mli' file in middle_end which currently have +x bits enabled for no good reason. Remark: To test this, playing with the DESTDIR variable is very useful (this lets you install to a new directory (or the same as before) without having to re-run the configure script). I used the following, fairly slow shell script to collect permissions: for f in $(find $DESTDIR); do \ echo $(basename $f) $(ls -l $f | cut -d' ' -f1); \ done | sort Remark: it is important to run `sync` in-between 'make install' runs to avoid timing effects due to filesystem or disk caching strategies. I believe that this corresponds to the natural time delay (and unrelated disk activity) that would occur in realistic change-install-test feedback loops.

(Suggestion made by Sébastien Hinderer during review.)

shindere · 2018-03-29T12:51:04Z

Gabriel Scherer (2018/03/29 05:37 -0700):

Thanks! The Mantis ticket is [MPR#5219](https://caml.inria.fr/mantis/print_bug_page.php?bug_id=5219).

Ah great, thanks for having ofund it out. It may have been assigned to me. Donot hesitate to assign it to you and to then close it once this is merged.

I performed the changes you suggested¹,

Thanks.

and also created a shared `Makefile.common` file to factorize all the `INSTALL_*` definitions duplicated in all the makefiles.

That's a very good idea. Thanks.

¹: except for one, I kept the letters instead of using octal modes, because I don't find the octal modes very usable, I never remember which action is which bit.

OK so IMO at the very least you should replace the + by an = so that the permissions are not relative to the ones in the repository, because if these are wrong,your + will propagate them to the installed files, which won't happen with an = Now, to be honest, I have always seen octal permissions in this context and I think they make sense to a lot of people. I will definitely not insist on that,though. In case it helps, the order of the bits is consistent with the order used by ls -l (rwx), so r(read) ir 4, w(write) is 2 and x(execution) is 1. I have to little requests re: your Makefile.common file: 1. Add a license header at the top 2. Please, when you define variables, use := rather than = and use spaces around the assignment operator.

gasche · 2018-03-29T13:11:01Z

I changed Makefile.common as advised -- it turns out this is the first time I create a file in the repository.

I used ?=-definitions for the INSTALL command(s) so that users can override them. I kept =-definitions for the INSTALL_*DIR variables because I believe that the weird shellquote trick in tools/Makefile relies on the fact that those definitions are expanded lazily.

shindere · 2018-03-29T14:46:53Z

Gabriel Scherer (2018/03/29 13:11 +0000):

I changed Makefile.common as advised -- it turns out this is the first time I create a file in the repository.

Thanks. Why didn't you take the coment on = vs. + in permissions?

I used `?=`-definitions for the `INSTALL` command(s) so that users can override them. I kept `=`-definitions for the INSTALL_*DIR variables because I believe that the weird `shellquote` trick in tools/Makefile relies on the fact that those definitions are expanded lazily.

OK.

gasche · 2018-03-29T14:59:04Z

Sorry, I had forgotten about the + vs. = suggestion, I just changed. (This does not change the actual behavior of the code, given that install always creates a fresh file with no initial permissions to add to, but it is clearer.)

(suggestion from Sébastien Hinderer)

shindere · 2018-03-29T15:05:20Z

Gabriel Scherer (2018/03/29 14:59 +0000):

Sorry, I had forgotten about the + vs. = suggestion, I just changed. (This does not change the actual behavior of the code, given that `install` always creates a fresh file with no initial permissions to add to, but it is clearer.)

Oh! I didn't know about that sutlety, sorry. I always use the u+x trick with chmod where it matters. :)

v-gb · 2018-07-02T10:46:44Z

This pull request changed the installation directory for ocamldoc. It used be be lib/ocaml/ocamldoc, now it's directly in lib/ocaml. This should probably be fixed before 4.07 is released?

gasche · 2018-07-02T10:48:23Z

Thanks for the catch... Yes, this sounds like something that we should fix, although I'd like to check with Damien before committing this late in the RC process.

gasche · 2018-07-02T10:50:27Z

ocamldoc/Makefile

 OCAMLDOC_LIBCMXA=odoc_info.cmxa
 OCAMLDOC_LIBA=odoc_info.$(A)

-INSTALL_LIBDIR=$(DESTDIR)$(LIBDIR)/ocamldoc


This line (in the before-the-patch system) caused the bug that @sliquister just reported; if we had used the standard definition for INSTALL_LIBDIR , and then explicitly $(INSTALL_LIBDIR)/ocaml later, or a differently-name DIR variable for a non-standard definition, then we wouldn't have silently changed the behavior.

The error comes from ocaml#1680, which changed the install scripts for ocamldoc, and was caught and reported by Valentin "sliquister" Gatien-Baron, presumably as part of his own work on ocaml#1569.

The error comes from #1680, which changed the install scripts for ocamldoc, and was caught and reported by Valentin "sliquister" Gatien-Baron, presumably as part of his own work on #1569.

The error comes from #1680, which changed the install scripts for ocamldoc, and was caught and reported by Valentin "sliquister" Gatien-Baron, presumably as part of his own work on #1569. This is the trunk counterpart of #1877 in 4.07, commit 9c9be40

gasche requested a review from shindere March 28, 2018 18:12

gasche force-pushed the install-install branch from c5b5a81 to cf195c7 Compare March 29, 2018 12:34

gasche added 4 commits March 29, 2018 14:40

Makefile: rename INSTALL_FLEXDLL into INSTALL_FLEXDLLDIR for consistency

3a0c7d9

ocamldoc/Makefile: rename variables for consistency

e08bb10

makefiles: turn the 'install' command into a variable

afcd29e

(Suggestion made by Sébastien Hinderer during review.)

gasche force-pushed the install-install branch from cf195c7 to 155ae88 Compare March 29, 2018 13:09

gasche force-pushed the install-install branch from 155ae88 to c2bd75f Compare March 29, 2018 14:57

gasche added 3 commits March 29, 2018 17:04

factorize common makefile definitions in Makefile.common

3702f53

Makefile.common: use = instead of + in modes for clarity

bfd3fb1

(suggestion from Sébastien Hinderer)

Changes entry

c2bd75f

shindere approved these changes Mar 29, 2018

View reviewed changes

gasche merged commit 6f52514 into ocaml:trunk Mar 29, 2018

gasche commented Jul 2, 2018

View reviewed changes

gasche mentioned this pull request Jul 2, 2018

fix an unintended change of library installation directory for ocamldoc #1877

Merged

vicuna mentioned this pull request Mar 14, 2019

use $(INSTALL) instead of 'cp' in Makefiles #5219

Closed

gasche mentioned this pull request Apr 17, 2020

configure/Makefile: Makefile.common is not .in anymore #9453

Closed

dra27 mentioned this pull request May 12, 2022

Eliminate $(BOOT_FLEXLINK_CMD) #11256

Merged

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

Update styles in learn area left sidebar (ocaml#1680)

a9b833c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makefiles: use 'install' instead of 'cp' in 'make install' targets#1680

makefiles: use 'install' instead of 'cp' in 'make install' targets#1680
gasche merged 7 commits intoocaml:trunkfrom
gasche:install-install

gasche commented Mar 28, 2018

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

gasche commented Mar 29, 2018 •

edited

Loading

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

gasche commented Mar 29, 2018

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

gasche commented Mar 29, 2018

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

v-gb commented Jul 2, 2018

Uh oh!

gasche commented Jul 2, 2018

Uh oh!

gasche Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gasche commented Mar 28, 2018

Why I care

Partial diagnosis

Change

Testing

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

gasche commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

gasche commented Mar 29, 2018

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

gasche commented Mar 29, 2018

Uh oh!

shindere commented Mar 29, 2018 via email

Uh oh!

v-gb commented Jul 2, 2018

Uh oh!

gasche commented Jul 2, 2018

Uh oh!

gasche Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gasche commented Mar 29, 2018 •

edited

Loading