Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from Mar 29, 2018

Conversation

Projects
None yet
3 participants
@gasche
Copy link
Member

gasche commented Mar 28, 2018

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.

@gasche gasche requested a review from shindere Mar 28, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 29, 2018

@gasche gasche force-pushed the gasche:install-install branch from c5b5a81 to cf195c7 Mar 29, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Mar 29, 2018

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.

gasche added some commits Mar 28, 2018

makefiles: use 'install' instead of 'cp' in 'make install' targets
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.
makefiles: turn the 'install' command into a variable
(Suggestion made by Sébastien Hinderer during review.)
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 29, 2018

@gasche gasche force-pushed the gasche:install-install branch from cf195c7 to 155ae88 Mar 29, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Mar 29, 2018

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

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 29, 2018

@gasche gasche force-pushed the gasche:install-install branch from 155ae88 to c2bd75f Mar 29, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Mar 29, 2018

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.)

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 29, 2018

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Jul 2, 2018

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

This comment has been minimized.

Copy link
Member Author

gasche commented Jul 2, 2018

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.

@@ -72,16 +74,9 @@ OCAMLDOC_LIBCMI=odoc_info.cmi
OCAMLDOC_LIBCMXA=odoc_info.cmxa
OCAMLDOC_LIBA=odoc_info.$(A)

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

This comment has been minimized.

@gasche

gasche Jul 2, 2018

Author Member

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.

gasche added a commit to gasche/ocaml that referenced this pull request Jul 2, 2018

fix an unintended change of library installation directory for ocamldoc
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.

damiendoligez added a commit that referenced this pull request Jul 3, 2018

fix an unintended change of library installation directory for ocamldoc
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.

gasche added a commit that referenced this pull request Jul 3, 2018

fix an unintended change of library installation directory for ocamldoc
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.