Improve performance by disabling repo tarring by rjbou · Pull Request #5015 · ocaml/opam

rjbou · 2022-01-21T15:43:12Z

~~that need a new name, ftm it is hideously named OPAMNOREPOTARRING~~
/cc @AltGr
fix #4586
edit:
By default disabled, environment variable OPAMREPOSITORYTARRING permit to enable it.

kit-ty-kate · 2022-01-21T17:56:36Z

I just tried #4586 (comment) on this PR and I get the exact same behaviour: /usr/bin/tar xfz /home/opam/.opam/repo/default.tar.gz -C /tmp/opam-5953-44547 is still called somewhere else

kit-ty-kate · 2022-01-21T17:58:14Z

Actually, the tar call appears a bit after the first archives retrivals if that helps.

kit-ty-kate

As mentioned in #4586 (comment) I think #3752 should be scraped entirely instead. An opt-in variable doesn’t make much sense in my opinion.

dra27 · 2022-01-28T10:51:31Z

From dev meeting: not arrived a name yet, but for 2.2 let's consider flipping this around the other way and disabling the optimisation by default and having the variable to opt in. Some benchmarking comparing the two on NVMe/SSD (i.e. "modern") systems before committing this would be handy.

rjbou · 2022-02-01T16:42:32Z

Updated!

src/client/opamArg.ml

rjbou · 2022-02-01T17:51:25Z

tests/reftests/init.test

 009e00fa
 ### OPAMYES=1
-### tar xzf ${OPAMROOT}/repo/default.tar.gz
+### cp -R ${OPAMROOT}/repo/default .


Should we untie this test from opam repo? @AltGr

kit-ty-kate

So fast!!! ❤️

I did some timings and here is what i got locally on my macOS system:
For opam update default (which is a local git repository):

master (5311f9b) takes 17 seconds
This PR takes 14 secondes
This PR (+ some manual tasks, see below) takes 6 seconds!!

In the third case I had to do some manual clean up tasks to get it to properly work and I think this is a bug in the PR:

$ ls ~/.opam/repo
alpha.tar.gz
default.tar.gz
lock
repos-config
state-205E9585.cache
$ opam update default
$ ls ~/.opam/repo
alpha.tar.gz
default.tar.gz
lock
repos-config
state-205E9585.cache

Here we can see that with this PR, opam does not remove the .tar.gz and simply use them as before (with some consistant improvements in time somehow). However as soon as I removed the .tar.gz files there, I got a really big x2 speedup and no tar.gz were reintroduced.

There must be something missing in this PR in its current state for opam to clean up the tar.gz if they already exist.

tests/reftests/repository.test

kit-ty-kate · 2022-02-02T11:08:53Z

The test also fails on Windows:

File "tests/reftests/repository.test", line 1, characters 0-0:
diff --git a/tests/reftests/repository.test b/tests/reftests/repository.out
old mode 100755
new mode 100644
index 1eac20e2..050b1233
--- a/tests/reftests/repository.test
+++ b/tests/reftests/repository.out
@@ -12,7 +12,7 @@ default
 lock
 repos-config
 ### opam install foo -vv | grep '^\+'
-+ ${BASEDIR}/OPAM/opam-init/hooks/sandbox.sh "build" "test" "-f" "bar" (CWD=${BASEDIR}/OPAM/optim/.opam-switch/build/foo.1)
++ /usr/bin/test "-f" "bar" (CWD=${BASEDIR}/OPAM/optim/.opam-switch/build/foo.1)
 ### opam remove foo
 The following actions will be performed:
   - remove foo 1
@@ -35,4 +35,4 @@ tarred.tar.gz
 + /usr/bin/tar "cfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz.tmp" "-C" "${OPAMTMP}" "tarred"
 ### opam install foo -vv | grep '^\+'
 + /usr/bin/tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}"
-+ ${BASEDIR}/OPAM/opam-init/hooks/sandbox.sh "build" "test" "-f" "bar" (CWD=${BASEDIR}/OPAM/optim/.opam-switch/build/foo.1)
++ /usr/bin/test "-f" "bar" (CWD=${BASEDIR}/OPAM/optim/.opam-switch/build/foo.1)
         git (internal) (exit 1)
(cd _build/default && /usr/bin/git --no-pager diff --no-index --color=always -u tests/reftests/repository.test tests/reftests/repository.out)

kit-ty-kate · 2022-02-02T19:21:28Z

Rebased for CI to start

kit-ty-kate · 2022-02-02T19:51:23Z

src/state/opamRepositoryState.ml

+      repositories
+  else
+    OpamRepositoryName.Map.iter (fun _name repo ->
+        OpamFilename.remove (OpamRepositoryPath.tar gt.root repo.repo_name))


I’m a bit surprised to see the remove being in lock instead of OpamUpdate.
I would expect only opam update to actually do anything.

I haven’t tried locally yet but unless I’ve missed something, I would expect such code to make opam crash for users calling opam install <pkg> just after having upgraded from opam 2.1 because they would have to get the content of the repository but it wouldn’t be available anywhere anymore.

But it’s just a hunch from reading the code, I’ll try locally later.

Yep it’s making it fail:

opam@feac57c1cc94:~/opam$ opam-2.1 install ocamlfind The following actions will be performed: - install ocamlfind 1.9.2 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> -> retrieved ocamlfind.1.9.2 (http://download.camlcity.org/download/findlib-1.9.2.tar.gz) -> installed ocamlfind.1.9.2 Done. opam@feac57c1cc94:~/opam$ ls ~/.opam/repo beta default lock repos-config state-0287C1DB.cache opam@feac57c1cc94:~/opam$ opam-2.1 update This development version of opam requires an update to the layout of /home/opam/.opam from version 2.0 to version 2.1, which can't be reverted. You may want to back it up before going further. Continue? [Y/n] y Format upgrade done. <><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><> [beta] no changes from git+https://github.com/ocaml/ocaml-beta-repository [default] synchronised from file:///home/opam/opam-repository <><> Synchronising development packages <><><><><><><><><><><><><><><><><><><><> opam@feac57c1cc94:~/opam$ ls ~/.opam/repo beta.tar.gz default.tar.gz lock repos-config state-0287C1DB.cache opam@feac57c1cc94:~/opam$ opam-2.1 remove ocamlfind The following actions will be performed: - remove ocamlfind 1.9.2 <><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><> -> removed ocamlfind.1.9.2 Done. opam@feac57c1cc94:~/opam$ ls ~/.opam/repo beta.tar.gz default.tar.gz lock repos-config state-0287C1DB.cache opam@feac57c1cc94:~/opam$ ./opam install ocamlfind [ERROR] No package named ocamlfind found. opam@feac57c1cc94:~/opam$ ls ~/.opam/repo lock repos-config state-0287C1DB.cache state-030998F6.cache

I believe it’s also making the opam-rt testsuite fail

Indeed, It is the wrong place

kit-ty-kate · 2022-02-02T23:51:33Z

I had a shot at fixing it: 1907fee I pushed it in your branch, feel free to remove it if you prefer another solution.
Here is the dockerfile I’ve used to show the new fixed behaviour:

FROM ocaml/opam:debian-11-ocaml-4.14
RUN git clone https://github.com/kit-ty-kate/opam -b disable-repo-optim
WORKDIR opam
RUN opam exec -- ./configure && opam exec -- make
RUN opam-2.1 update
RUN rm -rf ~/.opam
RUN opam-2.1 init --bare
RUN ls ~/.opam/repo
RUN ./opam switch create test --empty
RUN ./opam install conf-pkg-config
RUN ls ~/.opam/repo
RUN ./opam update
RUN ls ~/.opam/repo

as well as its corresonding steps 11 and 13:

Step 11/13 : RUN ls ~/.opam/repo
default.tar.gz
lock
repos-config
state-0287C1DB.cache
state-0F87227E.cache
Step 12/13 : RUN ./opam update

<><> Updating package repositories ><><><><><><><><><><><><><><><><><><><><><><>
[default] no changes from https://opam.ocaml.org
Step 13/13 : RUN ls ~/.opam/repo
default
lock
repos-config
state-0F87227E.cache

kit-ty-kate · 2022-02-03T15:33:18Z

tests/reftests/repository.test

+ /usr/bin/tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${OPAMTMP}"
+ /usr/bin/diff "-ruaN" "tarred" "tarred.new" (CWD=${OPAMTMP})
+ /usr/bin/tar "xfz" "${BASEDIR}/OPAM/repo/tarred.tar.gz" "-C" "${BASEDIR}/OPAM/repo/tarred"


It’s a bit wasteful to extract twice. Maybe we can do better

src/client/opamArg.ml

kit-ty-kate

To me it looks good to merge as-is (once CI is green of course)

I did some more tests in docker. Using this dockerfile:

FROM ocaml/opam:debian-11-ocaml-4.14
RUN git clone https://github.com/rjbou/opam -b disable-repo-optim
WORKDIR opam
RUN opam exec -- ./configure && opam exec -- make
RUN opam-2.1 update
RUN sudo apt install -yy time
RUN time opam-2.1 update
RUN ./opam update
RUN time ./opam update

With opam-2.1, opam update takes: 1m16s
With this PR, opam update takes: 45s

I’ve also tested on bare metal on a linux machine with /tmp mounted as tmpfs (15G) and I’m also getting a x2 performance improvement:

With opam 2.1, opam update takes: 16s
With this PR, opam update takes: 8s

(all the machines tested here have SSDs/NVMe)

🎉

dra27 · 2022-02-05T23:44:16Z

Fly-by comment: Cygwin's tar only deals in Cygwin paths, so even on Windows, these paths are translated to Unix versions, which is even more horrible than the present contortions we have to do with \ and /. The temporary directory appears here correctly in Unix format (i.e. as /tmp instead of C:\cygwin\tmp\) and is not going to be caught by the normal substitutions easily. Can the test instead do something to get rid of that path completely from the output?

kit-ty-kate · 2022-02-06T13:18:50Z

All green! Thanks a lot @dra27!

…ent variable `OPAMREPOSITORYTARRING'

…`OPAMREPOSITORYTARRING' is given This can be much faster on filesystems that don't cope well with scanning large trees but have good caching in /tmp. However this is slower in the general case Co-authored-by: Raja Boujbel <raja.boujbel@ocamlpro.com>

Co-authored-by: Kate <kit.ty.kate@disroot.org>

rjbou · 2022-02-09T11:36:25Z

Thanks all!

rjbou added this to the 2.2.0~alpha milestone Jan 21, 2022

rjbou requested a review from AltGr January 21, 2022 15:43

kit-ty-kate requested changes Jan 21, 2022

View reviewed changes

rjbou force-pushed the disable-repo-optim branch from b635713 to 8af59ab Compare February 1, 2022 16:42

rjbou requested a review from kit-ty-kate February 1, 2022 16:43

kit-ty-kate reviewed Feb 1, 2022

View reviewed changes

src/client/opamArg.ml Outdated Show resolved Hide resolved

rjbou commented Feb 1, 2022

View reviewed changes

kit-ty-kate requested changes Feb 1, 2022

View reviewed changes

kit-ty-kate reviewed Feb 1, 2022

View reviewed changes

tests/reftests/repository.test Outdated Show resolved Hide resolved

kit-ty-kate force-pushed the disable-repo-optim branch from 770c0ae to f01cc2d Compare February 2, 2022 19:20

rjbou requested a review from kit-ty-kate February 2, 2022 19:29

kit-ty-kate reviewed Feb 2, 2022

View reviewed changes

kit-ty-kate reviewed Feb 3, 2022

View reviewed changes

AltGr reviewed Feb 3, 2022

View reviewed changes

src/client/opamArg.ml Outdated Show resolved Hide resolved

kit-ty-kate approved these changes Feb 3, 2022

View reviewed changes

kit-ty-kate force-pushed the disable-repo-optim branch from 85f4f6a to e7f7b0d Compare February 4, 2022 01:46

kit-ty-kate force-pushed the disable-repo-optim branch from 60f3049 to 031930d Compare February 6, 2022 02:47

kit-ty-kate mentioned this pull request Feb 6, 2022

opam update: Avoid reloading repository contents when the repo has not changed #5043

Merged

rjbou and others added 3 commits February 8, 2022 17:27

repository config: add tarring configuration, retrieved from environm…

efa6e0b

…ent variable `OPAMREPOSITORYTARRING'

Only make a repo.tar.gz when repo_tarring is on with auto-upgrade

cf92d6e

rjbou and others added 3 commits February 8, 2022 18:03

reftests: update

dff641b

reftest: add repo archive storage tests

99a1595

Co-authored-by: Kate <kit.ty.kate@disroot.org>

update changes

8e42855

rjbou force-pushed the disable-repo-optim branch from c946ae2 to 8e42855 Compare February 8, 2022 17:10

kit-ty-kate changed the title ~~Disable repo optimistion with an environment variable~~ Disable repo tarring with an environment variable Feb 8, 2022

kit-ty-kate changed the title ~~Disable repo tarring with an environment variable~~ Improve performance by disabling repo tarring Feb 8, 2022

kit-ty-kate mentioned this pull request Feb 9, 2022

opam uses too much memory and gets killed #5050

Closed

rjbou merged commit 30bd998 into ocaml:master Feb 9, 2022

rjbou mentioned this pull request Apr 4, 2022

Fix repository loading #5109

Merged

rjbou mentioned this pull request May 30, 2023

opam var --package is slow on macos #5536

Closed

Conversation

rjbou commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kit-ty-kate commented Jan 21, 2022

Uh oh!

kit-ty-kate commented Jan 21, 2022

Uh oh!

kit-ty-kate left a comment

Choose a reason for hiding this comment

Uh oh!

dra27 commented Jan 28, 2022

Uh oh!

rjbou commented Feb 1, 2022

Uh oh!

Uh oh!

rjbou Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kit-ty-kate commented Feb 2, 2022

Uh oh!

kit-ty-kate commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kit-ty-kate Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

rjbou Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate commented Feb 2, 2022

Uh oh!

kit-ty-kate Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kit-ty-kate left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dra27 commented Feb 5, 2022

Uh oh!

kit-ty-kate commented Feb 6, 2022

Uh oh!

rjbou commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rjbou commented Jan 21, 2022 •

edited

Loading

kit-ty-kate commented Feb 2, 2022 •

edited

Loading

kit-ty-kate left a comment •

edited

Loading