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

Cache file contents in action builder by name. #6555

Merged

Conversation

dkalinichenko-js
Copy link
Collaborator

Improve overall performance in builds by removing redundant reads. Previously, Dune was caching file contents only by callsites.

Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Thanks! Please sign your commit, by adding a line like this (but with your name/email) into the commit message:

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>

@snowleopard
Copy link
Collaborator

snowleopard commented Nov 24, 2022

It's worth adding that in our monorepo, this change sped up null builds by the factor of 2.6x! We were reading every file 6 times on average.

@ejgallego Could you check if this change makes Coq null builds noticeably faster? (Asking because presumably Coq build is pretty expensive.)

@snowleopard
Copy link
Collaborator

@dkalinichenko-js There is a little benchmark in perf.sh. Could you run it and show the results?

Signed-off-by: Diana Kalinichenko <dkalinichenko@janestreet.com>
@dkalinichenko-js
Copy link
Collaborator Author

Thanks! Please sign your commit, by adding a line line this (but with your name/email) into the commit message:

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>

Oops, done.

@Alizter
Copy link
Collaborator

Alizter commented Nov 24, 2022

@snowleopard our monorepo coq-universe needs some work since we are updating the Coq version, but I'll make a note to test ASAP when it is functional again.

@dkalinichenko-js
Copy link
Collaborator Author

@dkalinichenko-js There is a little benchmark in perf.sh. Could you run it and show the results?

Unfortunately can't get the benchmark to compile on my machine, but I don't expect much improvement outside of Jane due to different build rules. Merging anyway.

@dkalinichenko-js dkalinichenko-js merged commit c9ebe06 into ocaml:main Nov 24, 2022
@dkalinichenko-js dkalinichenko-js deleted the cache-file-contents-by-name branch November 24, 2022 15:15
@snowleopard
Copy link
Collaborator

snowleopard commented Nov 24, 2022

@snowleopard our monorepo coq-universe needs some work since we are updating the Coq version, but I'll make a note to test ASAP when it is functional again.

@Alizter Thanks, please share the results!

jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 24, 2022
* main: (58 commits)
  test: formatting of alternative dune files (ocaml#6567)
  refactor: remove Modules.is_empty (ocaml#6564)
  refactor: module kinds (ocaml#6562)
  refactor(coq): resolve lack of coqc properly
  Cache file contents in action builder by name. (ocaml#6555)
  fix: re-enable dune on older macos sdk's (ocaml#6515)
  fix: do not hide lib interface module (ocaml#6549)
  test: remove pkg-config output for reproducibility (ocaml#6543)
  melange: add test for ocaml flags (ocaml#6548)
  fix: improve virtual library error messages
  test: virtual library and impl locations
  test: alias module regression (ocaml#6544)
  refactor(merlin): dump config sub command (ocaml#6547)
  refactor: simplify merlin (ocaml#6508)
  chore(nix): use nix-overlays for the slim devShell (ocaml#6546)
  fix: module compilation rule env (ocaml#6527)
  chore: update nix (ocaml#6536)
  fix: merlin rules with pp's (ocaml#6474)
  Call [Dune_util.Log.init] as soon as possible (ocaml#6542)
  refactor: speed up stdlib build (ocaml#6524)
  ...
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 24, 2022
* main:
  test: formatting of alternative dune files (ocaml#6567)
  refactor: remove Modules.is_empty (ocaml#6564)
  refactor: module kinds (ocaml#6562)
  refactor(coq): resolve lack of coqc properly
  Cache file contents in action builder by name. (ocaml#6555)
  fix: re-enable dune on older macos sdk's (ocaml#6515)
  fix: do not hide lib interface module (ocaml#6549)
  test: remove pkg-config output for reproducibility (ocaml#6543)
  melange: add test for ocaml flags (ocaml#6548)
  fix: improve virtual library error messages
  test: virtual library and impl locations
  test: alias module regression (ocaml#6544)
  refactor(merlin): dump config sub command (ocaml#6547)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants