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

Make install faster by not scanning the whole switch when no install field is present #4494

Merged
merged 5 commits into from
Apr 18, 2021

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Jan 8, 2021

Implements #4422

Currently opam scans the whole switch directory recursively twice when installing a package (per package). This introduces a huge strain on the IO of the machine. This is especially visible under high load (e.g. in CI)

This PR makes so that opam only scan the files that are going to be installed (if they exist already, this is rare) if the package does not have any install field. If the package does have an install field the old behaviour is used.

@kit-ty-kate kit-ty-kate marked this pull request as ready for review January 14, 2021 17:20
@kit-ty-kate kit-ty-kate changed the title [WIP] Make install faster by not scanning the whole switch when no install field is present Make install faster by not scanning the whole switch when no install field is present Jan 14, 2021
@kit-ty-kate
Copy link
Member Author

This PR is now ready for review. cc @AltGr

@dra27 dra27 added this to PR in Progress in Opam 2.1.x via automation Jan 22, 2021
@kit-ty-kate kit-ty-kate moved this from PR in Progress to PR Finalised in Opam 2.1.x Feb 6, 2021
@dra27 dra27 added this to the 2.1.0~rc milestone Feb 12, 2021
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more complicated than what I would have thought at first; I was initially pretty confused by track_files comments saying it is "similar to [track]", while the latter is a wrapper around the execution of a job and the former just an atomic function that expects the results of a previous run as argument; there is nothing wrong with that, but what were the reasons not to go for the more straight-forward extraction of the list of files from .install, followed by the run of a version of track that is limited to a given set of files ? It would be useful to document more precisely the usage of track_files in opamDirTrack.mli.

Another remark, it seems the behaviour on pre-install hooks is changed, since they are no longer tracked. Have you studied if that had any consequences in the wild ? Though the only other option would be to disable the optimised tracking as soon as any pre-install hook is defined, I guess... The tracking behaviour for post-install is already specific and detailed here.

I am otherwise happy with the code and this will be nice to have!

@rjbou rjbou removed this from the 2.1.0~rc milestone Feb 26, 2021
@rjbou rjbou moved this from PR Finalised to PR in Progress in Opam 2.1.x Feb 26, 2021
@rjbou rjbou self-assigned this Mar 24, 2021
@rjbou rjbou requested a review from AltGr March 30, 2021 16:36
@rjbou
Copy link
Collaborator

rjbou commented Apr 2, 2021

Just to keep a trace of the test (can't be added in reftest):

$ cat dotty/dot.install
bin: [ "script" ]
$ cat dotty/dot.opam
opam-version: "2.0"
build: [ "sh" "-c" "echo hellow > script%{exe}%" ]
$ opam pin -yn ./dotty
$ opam install dot #with win executable name
<><> Synchronising pinned packages ><><><><><><><><><><><><><><><><><><><><><><>
[dot.0.1] synchronised (no changes)

The following actions will be performed:
  ∗ install dot 0.1*

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
[WARNING] .install file is missing .exe extension for script
[WARNING] Automatically adding .exe to <switchprefix>/build/dot.0.1/script.exe
∗ installed dot.0.1
Done.

@rjbou rjbou moved this from PR in Progress to PR To review in Opam 2.1.x Apr 9, 2021
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a teeny bit of nitpicking, otherwise all good! ;)

master_changes.md Outdated Show resolved Hide resolved
src/core/opamDirTrack.mli Outdated Show resolved Hide resolved
Opam 2.1.x automation moved this from PR To review to PR Finalised Apr 16, 2021
rjbou and others added 5 commits April 16, 2021 15:51
…` instead of the whole switch prefix when there are no `install:` instructions (and no preinstall commands)

Co-authored-by:     Kate <kit.ty.kate@disroot.org>
@rjbou rjbou merged commit 2a8f23f into ocaml:master Apr 18, 2021
Opam 2.1.x automation moved this from PR Finalised to Done Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Opam 2.1.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants