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

fix(coq): Fix performance regression in coqdep unescaping #10115

Merged
merged 1 commit into from Feb 22, 2024

Conversation

ejgallego
Copy link
Collaborator

Regression from #9231

Fixes #10088

Regression from ocaml#9231

Fixes ocaml#10088

Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
@ejgallego ejgallego added the coq label Feb 22, 2024
@ejgallego
Copy link
Collaborator Author

ejgallego commented Feb 22, 2024

With this PR, zero build for Coq-HoTT:

main:
real	0m7,924s

this PR:
real	0m5,952s
user	0m5,857s
sys	0m0,083s

3.11.1:
real	0m5,543s
user	0m5,420s
sys	0m0,132s
egallego@mobiverse:~/external/coq/HoTT$ time ~/external/dune/_build/install/default/bin/dune build

so it seems like reasonable as to consider the original regression closed.

I was curious and I did time the .v.d file parsing as to understand what was the total for the zero build.

And indeed, it seems we are not memoizing the parsing of the .v.d. file, that's gonna be terrible! (cc: @Alizter )

I seem to recall we had this properly covered, I wonder what happened? But def we should store the Dep_map into a memo node.

@ejgallego
Copy link
Collaborator Author

This PR is pretty trivial so I will merge soon, anyways the regression fixed is not observable after #10116 is merged, which solves the real culprit.

@ejgallego ejgallego merged commit 6d741e0 into ocaml:main Feb 22, 2024
26 of 27 checks passed
@ejgallego ejgallego deleted the fix_perf_coqdep_parsing branch February 22, 2024 16:24
rlepigre pushed a commit to rlepigre/dune that referenced this pull request Feb 29, 2024
fix(coq): Fix performance regression in coqdep unescaping
rlepigre pushed a commit to rlepigre/dune that referenced this pull request Feb 29, 2024
fix(coq): Fix performance regression in coqdep unescaping
rlepigre pushed a commit to rlepigre/dune that referenced this pull request Feb 29, 2024
fix(coq): Fix performance regression in coqdep unescaping
@emillon emillon mentioned this pull request Mar 4, 2024
15 tasks
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 11, 2024
CHANGES:

### Fixed

- When a directory is changed to a file, correctly remove it in subsequent
  `dune build` runs. (ocaml/dune#9327, fix ocaml/dune#6575, @emillon)

- Fix a problem with the doc-new target where transitive dependencies were
  missed during compile. This leads to missing expansions in the output docs.
  (ocaml/dune#9955, @jonludlam)

- coq: fix performance regression in coqdep unescaping (ocaml/dune#10115, fixes ocaml/dune#10088,
  @ejgallego, thanks to Dan Christensen for the report)

- coq: memoize coqdep parsing, this will reduce build times for Coq users, in
  particular for those with many .v files (ocaml/dune#10116, @ejgallego, see also ocaml/dune#10088)

- on Windows, use an unicode-aware version of `CreateProcess` to avoid crashes
  when paths contains non-ascii characters. (ocaml/dune#10212, fixes ocaml/dune#10180, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[performance] Zero-build regression 3.11 - 3.14
1 participant