CP-51479: Optimize 'make install' #5982

edwintorok · 2024-09-11T17:22:28Z

We spend quite a lot of time here because dune is invoked multiple times from different makefile rules, also install.sh was very verbose.

Draft PR to check github CI.

Running make build install with no changes to the code is ~3x faster now:

Benchmark 1: rm /tmp/install/1 -rf; make -C ../scm-master build install DESTDIR=/tmp/install/1
  Time (mean ± σ):     20.575 s ±  0.239 s    [User: 13.093 s, System: 9.113 s]
  Range (min … max):   20.356 s … 21.064 s    10 runs

Benchmark 2: rm /tmp/install/2 -rf; make build install DESTDIR=/tmp/install/2
  Time (mean ± σ):      6.355 s ±  0.954 s    [User: 7.489 s, System: 5.320 s]
  Range (min … max):    5.827 s …  9.040 s    10 runs

Summary
  rm /tmp/install/2 -rf; make build install DESTDIR=/tmp/install/2 ran
    3.24 ± 0.49 times faster than rm /tmp/install/1 -rf; make -C ../scm-master build install DESTDIR=/tmp/install/1

20s for a noop incremental build was too slow, 6s is still slow, but better.

And running just make install when there are no changes is about ~7x faster now:

Benchmark 1: rm /tmp/install/2 -rf; make install DESTDIR=/tmp/install/2
  Time (mean ± σ):      2.791 s ±  0.075 s    [User: 4.469 s, System: 4.640 s]
  Range (min … max):    2.591 s …  2.863 s    10 runs

Koji build times are slightly improved too (~3m44s for buildArch phase vs ~4m06s).

There are some changes in file permissions, but they look OK (e.g. having execute permissions on the manpages was wrong to begin with):

Permission changes

``` --- refs 2024-09-16 09:24:17.408911755 +0100 +++ thiss 2024-09-16 09:24:17.415912441 +0100 @@ -1404,7 +1404,7 @@ ./opt/xensource/bin/rrd2csv -rwxr-xr-x ./opt/xensource/bin/static-vdis -rwxr-xr-x ./opt/xensource/bin/update-ca-bundle.sh -rwxr-xr-x -./opt/xensource/bin/xapi -r-xr-xr-x +./opt/xensource/bin/xapi -rwxr-xr-x ./opt/xensource/bin/xapi-autostart-vms -rwxr-xr-x ./opt/xensource/bin/xapi-db-process -rwxr-xr-x ./opt/xensource/bin/xapi-wait-init-complete -rwxr-xr-x @@ -1422,13 +1422,13 @@ ./opt/xensource/bin/xe-switch-network-backend -rwxr-xr-x ./opt/xensource/bin/xe-toolstack-restart -rwxr-xr-x ./opt/xensource/bin/xe-xentrace -rwxr-xr-x -./opt/xensource/bin/xsh -r-xr-xr-x +./opt/xensource/bin/xsh -rwxr-xr-x ./opt/xensource/debug/debug_ha_query_liveset -rwxr-xr-x ./opt/xensource/debug/event_listen -rwxr-xr-x ./opt/xensource/debug/import-update-key -rwxr-xr-x ./opt/xensource/debug/perftest -rwxr-xr-x ./opt/xensource/debug/quicktest -rwxr-xr-x -./opt/xensource/debug/quicktestbin -r-xr-xr-x +./opt/xensource/debug/quicktestbin -rwxr-xr-x ./opt/xensource/debug/rbac_static.csv -rw-r--r-- ./opt/xensource/debug/suspend-image-viewer -rwxr-xr-x ./opt/xensource/debug/vncproxy -rwxr-xr-x @@ -1459,7 +1459,7 @@ ./opt/xensource/libexec/network-init -rwxr-xr-x ./opt/xensource/libexec/pbis-force-domain-leave -rwxr-xr-x ./opt/xensource/libexec/print-custom-templates -rwxr-xr-x -./opt/xensource/libexec/python_nbd_client.py -rw-r--r-- +./opt/xensource/libexec/python_nbd_client.py -rwxr-xr-x ./opt/xensource/libexec/reset-and-reboot -rwxr-xr-x ./opt/xensource/libexec/restore-sr-metadata.py -rwxr-xr-x ./opt/xensource/libexec/save-boot-info -rwxr-xr-x @@ -1705,9 +1705,9 @@ ./usr/sbin/xenopsd-simulator -rwxr-xr-x ./usr/sbin/xenopsd-xc -rwxr-xr-x ./usr/share/man/man1/xcp-networkd.1 -rw-r--r-- -./usr/share/man/man1/xenops-cli.1.gz -rwxr-xr-x -./usr/share/man/man1/xenopsd-simulator.1.gz -rwxr-xr-x -./usr/share/man/man1/xenopsd-xc.1.gz -rwxr-xr-x +./usr/share/man/man1/xenops-cli.1.gz -rw-r--r-- +./usr/share/man/man1/xenopsd-simulator.1.gz -rw-r--r-- +./usr/share/man/man1/xenopsd-xc.1.gz -rw-r--r-- ./usr/share/man/man8/xapi-storage-script.8 -rw-r--r-- ./usr/share/xapi/doc/classes.dot -rw-r--r-- ./usr/share/xapi/doc/doc-convert.sh -rwxr-xr-x @@ -2034,7 +2034,7 @@ ./usr/share/xapi/doc/markdown/class-vtpm.md -rw-r--r-- ./usr/share/xapi/doc/markdown/class-vusb.md -rw-r--r-- ./usr/share/xapi/doc/markdown/classes.md -rw-r--r-- -./usr/share/xapi/doc/markdown/management-api.md -r--r--r-- +./usr/share/xapi/doc/markdown/management-api.md -rw-r--r-- ./usr/share/xapi/doc/markdown/relationships-between-classes.md -rw-r--r-- ./usr/share/xapi/doc/markdown/toc.yml -rw-r--r-- ./usr/share/xapi/doc/markdown/types.md -rw-r--r-- ```

ocaml/gencert/dune

Makefile

psafont · 2024-09-18T10:28:10Z

It shouldn't need changes in xapi.spec, all I did was a xenpkg specsync and it built (there are other changes that would require that, but I haven't opened a PR with that as well, in this PR I've added some extra lines to the Makefile to keep the list of files backwards compatible).

Good to know, with many packages being changed, I would have expected that the rpm packages would have to change as well

…l.in ./configure generates api_version.ml from api_version.ml.in, but their comments didn't match. Synchronize them, such that running ./configure doesn't result in committable changes. Also ignore the api_version.ml.in2 file that gets created. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

We cannot prevent dune from building them at the moment (`nodynlink` is disabled, and doesn't work with PIE executables). However disable installing all these files to avoid the .spec file complaining about additional cmxs files appearing when opam packages are reorganized. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

'update-dm-lifecycle' and 'python' only exists in a single directory, tell 'dune' exactly where to look. This speeds up incremental builds: ``` dune build @ocaml/xapi-storage/python/xapi/storage/api/v5/python --profile=release ran 3.41 ± 0.18 times faster than dune build @python --profile=release dune build @ocaml/idl/update-dm-lifecycle -j 8 --profile=release ran 2.13 ± 0.19 times faster than dune build @update-dm-lifecycle -j 8 --profile=release --auto-promote ``` ``` Benchmark 2: dune build @ocaml/xapi-storage/python/xapi/storage/api/v5/python --profile=release Time (mean ± σ): 288.7 ms ± 3.7 ms [User: 229.5 ms, System: 58.9 ms] Range (min … max): 282.5 ms … 293.5 ms 10 runs Benchmark 2: dune build @ocaml/idl/update-dm-lifecycle -j 8 --profile=release Time (mean ± σ): 581.4 ms ± 15.8 ms [User: 442.5 ms, System: 175.8 ms] Range (min … max): 550.5 ms … 602.2 ms 10 runs ``` Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Use `opam-dune-lint`. This will make it easier to later merge opam packages and keep the merged package updated correctly. Although `opam-dune-lint` doesn't find all problems yet, see: ocurrent/opam-dune-lint#71 Signed-off-by: Edwin Török <edwin.torok@cloud.com>

`set -x` is only needed when debugging the script. Don't flood 'make install' output. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

…ucceed Previously it failed because the symlink already existed. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

…re writing files When run inside Dune's sandbox you cannot rely on another rule having created the empty directories for you. Ensure that we create parent directories before we write a new file, otherwise we fail with a `Sys_error` about `No such file or directory`. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Also add an 'alias generate' for the csharp/autogen/LICENSE file. All other sdk subdirs have a 'generate' alias in 'autogen', except for 'csharp', and without it 'make sdk' would fail to build due to the missing file. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Avoid copying twice, get dune to install the files to the correct destination in one go. Also add a copy of XE_SR_ERRORCODES.xml, by default 'make install' would look for this in /opt/xensource, and writing there requires root. (This can be overriden with `./configure --share`). Since we are using `dune` to install the files now we need the file to always be present. Had to adjust the paths used by the CI. Uses dune directory targets, and the directory must be entirely under the control of these rules. There are some static files in autogen/ though, so move the generated ones to autogen-out, and then use 'cp -r' to copy over the static ones (there is no builtin dune action for the copy, there are individual copy actions, or a copy_files rule, but neither is suitable here) Signed-off-by: Edwin Török <edwin.torok@cloud.com>

To avoid the .spec build complaining about newly installed files. These aren't used outside of XAPI anyway. Remove test on xenopsd.cmxs, which is no longer built (there is still a test on the simulator and main XAPI). Signed-off-by: Edwin Török <edwin.torok@cloud.com>

…i-doc rule More efficient than install.sh. Also enables merging opam packages more easily, as it makes it clearer which package installs files where. 'xapi' and 'xe' are special cases because they install to /opt/xensource, so add a separate line with a different --prefix for them. We need a new xapi-debug package to install to /opt/xensource/debug which is a non-FHS compliant path, and we override its bindir to be /opt/xensource/debug. `vhd-tool` is special too, since it installs some files to /usr/libexec/xapi, but xapi itself is in /opt/xensource and its libexec would be /opt/xensource/libexec. Dune doesn't support installing to arbitrary directories, there is a 'misc' rule for that, but it is deprecated so I wouldn't start using it now. These rules generate multiple files in a directory, use dune's directory target support. Previously we would've copied the files multiple times. This is quite difficult to review, I suggest reviewing the outcome of the PR: On b123832 (do not installs cmxs commit): ``` rm /tmp/inst/1 -rf; make build install DESTDIR=/tmp/inst/1 && (cd /tmp/inst/1 && find ! -type d -printf '%p %M\n' ) >|ref ``` On the final commit of this PR: ``` rm /tmp/inst/2 -rf; make build install DESTDIR=/tmp/inst/2 && (cd /tmp/inst/2 && find ! -type d -printf '%p %M\n' ) >|this && sort -u ref >|refs && sort -u this >|thiss && diff -wu refs thiss ``` Signed-off-by: Edwin Török <edwin.torok@cloud.com>

compare with: ``` rm /tmp/inst/2 -rf; make build install DESTDIR=/tmp/inst/2 && (cd /tmp/inst/2 && find ! -type d -printf '%p %M %8s\n' ) >|this && sort -u ref >|refs && sort -u this >|thiss && diff -wu refs thiss && echo OK ``` Signed-off-by: Edwin Török <edwin.torok@cloud.com>

The dune build runs can't be parallelized, but the python and install.sh invocations and the `dune install` invocations can. Also remove the dependency between 'build' and 'install', if desired during development one can run `make build install`, however for the .spec build it is better to have a strict separation between build and install phases (this ensures that the install phase doesn't rebuild things by accident). Signed-off-by: Edwin Török <edwin.torok@cloud.com>

…i-rrdd-plugin They use the same install destination. xapi-rrdd-plugin was never installed, so remove the public-name and opam package (but keep the sources, they might be useful as an example). TODO: opam dune lint Signed-off-by: Edwin Török <edwin.torok@cloud.com>

These weren't being installed previously during a spec build, and had to be 'rm'-ed in the Makefile. Join them into a single package instead to reduce the number of opam files. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Signed-off-by: Edwin Török <edwin.torok@cloud.com>

Improves: 1bb4543 ("CP-51479: [maintenance]: install SDK files using dune rules") Signed-off-by: Edwin Török <edwin.torok@cloud.com>

xs-opam build was failing due to missing dep between ezxenstore and xapi-stext-unix. opam-dune-lint didn't spot this one. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

This doesn't appear to be epoll related but causes an xs-opam failure. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

edwintorok · 2024-09-23T08:54:20Z

The earlier dependency failure here was because newly added packages get a version of ~dev, but everything else we have in opam is declared as master, and then the = version constraint can't find a match between rrd-transport (master) and xapi-tools (~dev).
I removed the = version constraint from rrd-transport for now.
(Even if you opam pin the other packages, they get a version of master because that is what xs-opam specified, pinning doesn't override the version).

Although next time we update xs-opam we should see whether using ~dev instead of master might be better.

psafont

I'd like to wait to merge this after the epoll version has been tagged

edwintorok changed the title ~~Optimize 'make install'~~ Optimize 'make install' [CI only, ignore for now] Sep 11, 2024

edwintorok force-pushed the private/edvint/packageopt0 branch 5 times, most recently from 8c6884a to b8387ca Compare September 12, 2024 09:42

psafont reviewed Sep 12, 2024

View reviewed changes

ocaml/gencert/dune Outdated Show resolved Hide resolved

psafont reviewed Sep 12, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

edwintorok force-pushed the private/edvint/packageopt0 branch 18 times, most recently from e5d4412 to 7cc042f Compare September 16, 2024 08:50

edwintorok changed the title ~~Optimize 'make install' [CI only, ignore for now]~~ CP-51479: Optimize 'make install' Sep 16, 2024

edwintorok force-pushed the private/edvint/packageopt0 branch 2 times, most recently from d3d0ef2 to 137753f Compare September 16, 2024 09:38

edwintorok mentioned this pull request Sep 16, 2024

Update feature/perf from master #5993

Closed

edwintorok force-pushed the private/edvint/packageopt0 branch from 899ce3e to f9764d0 Compare September 19, 2024 09:25

edwintorok added 18 commits September 19, 2024 18:40

CP-51479: [maintenance]: make install.sh more quiet

cee8ed6

`set -x` is only needed when debugging the script. Don't flood 'make install' output. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

CP-51479: [maintenance]: running make install twice in a row should s…

f465a55

…ucceed Previously it failed because the symlink already existed. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

CP-51479: [maintenance]: use common dune install and uninstall variables

8956377

Signed-off-by: Edwin Török <edwin.torok@cloud.com>

CP-51479: [maintenance]: use 'if' instead of boolean logic

4ce9906

Improves: 1bb4543 ("CP-51479: [maintenance]: install SDK files using dune rules") Signed-off-by: Edwin Török <edwin.torok@cloud.com>

CP-51479: [maintenance]: fix opam dependencies after epoll merge

0a8fc6e

xs-opam build was failing due to missing dep between ezxenstore and xapi-stext-unix. opam-dune-lint didn't spot this one. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

edwintorok force-pushed the private/edvint/packageopt0 branch from f9764d0 to 0a8fc6e Compare September 19, 2024 17:47

CP-51479: [maintenance]: fix opam dependency on rrd-transport

48f12a7

This doesn't appear to be epoll related but causes an xs-opam failure. Signed-off-by: Edwin Török <edwin.torok@cloud.com>

edwintorok force-pushed the private/edvint/packageopt0 branch from 85ffeb4 to 48f12a7 Compare September 23, 2024 08:54

psafont approved these changes Sep 23, 2024

View reviewed changes

edwintorok enabled auto-merge September 26, 2024 09:40

robhoes approved these changes Oct 1, 2024

View reviewed changes

edwintorok added this pull request to the merge queue Oct 1, 2024

Merged via the queue into xapi-project:master with commit 9e1ea63 Oct 1, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CP-51479: Optimize 'make install' #5982

CP-51479: Optimize 'make install' #5982

Uh oh!

edwintorok commented Sep 11, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

psafont commented Sep 18, 2024

Uh oh!

edwintorok commented Sep 23, 2024 •

edited

Loading

Uh oh!

psafont left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CP-51479: Optimize 'make install' #5982

CP-51479: Optimize 'make install' #5982

Uh oh!

Conversation

edwintorok commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psafont commented Sep 18, 2024

Uh oh!

edwintorok commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psafont left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edwintorok commented Sep 11, 2024 •

edited

Loading

edwintorok commented Sep 23, 2024 •

edited

Loading