Skip to content

perf: run parse_compilation_units once#7187

Merged
rgrinberg merged 12 commits intoocaml:mainfrom
jchavarri:perf/run-parse_compilation_units-once
Mar 3, 2023
Merged

perf: run parse_compilation_units once#7187
rgrinberg merged 12 commits intoocaml:mainfrom
jchavarri:perf/run-parse_compilation_units-once

Conversation

@jchavarri
Copy link
Copy Markdown
Collaborator

@jchavarri jchavarri commented Feb 26, 2023 •
edited
Loading

parse_compilation_units can be expensive when there are a lot of modules in the map. This PR makes sure it only runs once per lib (or other stanzas).

Some measurements for a few actions in Ahrefs repo. All actions are "noops" (the command was run previously successfully so no user actions should be executed):

Command main This branch
dune exec gen_esgg real 0m10.121s, user 0m9.677s, sys 0m0.440s real 0m1.372s, user 0m1.157s, sys 0m0.216s
dune build @fe.all real 0m32.039s, user 0m30.391s, sys 0m1.685s real 0m12.716s, user 0m11.615s, sys 0m1.119s
dune build @gen_dune real 0m25.436s, user 0m24.253s, sys 0m1.223s real 0m5.407s, user 0m4.908s, sys 0m0.504s

TODO

@jchavarri
perf: run parse_compilation_units once
c9c2e90 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri self-assigned this Feb 26, 2023
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Feb 26, 2023

Fix looks right. I'm wondering if we could simplify things even further though. How about saving the object map as part of Modules.t? This would remove a lot of the staging as well.

@jchavarri
Merge branch 'main' into perf/run-parse_compilation_units-once
e1386e5 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
benchmark: run in branch
cbc3621 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Copy link
Copy Markdown
Collaborator Author

jchavarri commented Feb 28, 2023 •
edited
Loading

@rgrinberg how do you think the types should be modified in order to move in that direction? Something like this?

type t =
  { obj_map : Module.t Module_name.Unique.Map.t
  ; modules : modules
  }

and modules =
  | Singleton of Module.t
  | Unwrapped of Unwrapped.t
  | Wrapped of Wrapped.t
  | Impl of impl
  | Stdlib of Stdlib.t

and impl =
  { impl : t
  ; vlib : t
  }

@jchavarri
Copy link
Copy Markdown
Collaborator Author

jchavarri commented Feb 28, 2023

Another thing I wonder about the idea of saving the object map as part of Modules.t is if there can be cases where this object map would be created unnecessarily, in a wasteful way 🤔

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Feb 28, 2023

how do you think the types should be modified in order to move in that direction? Something like this?

Yeah that sounds good.

Another thing I wonder about the idea of saving the object map as part of Modules.t is if there can be cases where this object map would be created unnecessarily, in a wasteful way 🤔

You could make it lazy to avoid that.

jchavarri added 2 commits March 1, 2023 20:56
@jchavarri
move unique_map to modules
edf5724 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Merge branch 'main' into perf/run-parse_compilation_units-once
8ee7650 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
jchavarri
jchavarri commented Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

@rgrinberg please take a look to see if this is going in the right direction, thanks!

@jchavarri
update Modules.equal
9595e70 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri requested a review from rgrinberg March 1, 2023 22:26
rgrinberg
rgrinberg reviewed Mar 1, 2023
View reviewed changes
rgrinberg
rgrinberg reviewed Mar 1, 2023
View reviewed changes
rgrinberg
rgrinberg reviewed Mar 1, 2023
View reviewed changes
rgrinberg
rgrinberg reviewed Mar 1, 2023
View reviewed changes
rgrinberg
rgrinberg reviewed Mar 1, 2023
View reviewed changes
rgrinberg
rgrinberg reviewed Mar 1, 2023
View reviewed changes
@jchavarri
refactor: simplify Modules.obj_map
955fab5 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Copy link
Copy Markdown
Collaborator Author

jchavarri commented Mar 2, 2023

@rgrinberg thanks for the review, I think I tackled all your comments, the code looks better now.

If it looks good to merge, before doing so please take a look at #7198. I would like to merge that benchmark PR first, so that we can make measurements compare and understand better the impact in warm builds, thanks!

@jchavarri jchavarri marked this pull request as ready for review March 2, 2023 15:50
@jchavarri jchavarri requested a review from rgrinberg March 2, 2023 15:50
@jchavarri jchavarri mentioned this pull request Mar 2, 2023
Merged
rgrinberg
rgrinberg reviewed Mar 2, 2023
View reviewed changes
src/dune_rules/modules.mli Outdated
end

val obj_map : t -> f:(Sourced_module.t -> 'a) -> 'a Module.Obj_map.t
val obj_map : t -> Module.t Module_name.Unique.Map.t
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Mar 2, 2023

Choose a reason for hiding this comment

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

Should we just make this valued by Sourced_module.t? It doesn't really hurt for ocamldep, and it will remove further redundant computations.

Copy link
Copy Markdown
Collaborator Author

@jchavarri jchavarri Mar 2, 2023

Choose a reason for hiding this comment

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

Do you mean the type being

val obj_map : t -> Sourced_module.t Module_name.Unique.Map.t

?
I tried something like that in c8e81be, but then I get errors in deps_of_module and deps_of_vlib_module because of the change, as some functions like Modules.alias_for still operate on Module.t.

Copy link
Copy Markdown
Collaborator Author

@jchavarri jchavarri Mar 2, 2023

Choose a reason for hiding this comment

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

File "src/dune_rules/dep_rules.ml", line 62, characters 16-20:
62 |       aliases @ deps)
                     ^^^^
Error: This expression has type Modules.Sourced_module.t list
       but an expression was expected of type Module.t list
       Type Modules.Sourced_module.t is not compatible with type Module.t

Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Mar 2, 2023

Choose a reason for hiding this comment

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

Right, but you can extract the Module.t out of the Sourced_module.t without too many issues.

Copy link
Copy Markdown
Collaborator Author

@jchavarri jchavarri Mar 2, 2023

Choose a reason for hiding this comment

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

This builds, but I'm not sure I got the idea right, as it seems more complex than before 🤔 559e885

Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Mar 2, 2023

Choose a reason for hiding this comment

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

I think you can simplify it by keeping ocamldep.mli the same, and moving the to_module conversion to ocamldep.ml.

Copy link
Copy Markdown
Collaborator Author

@jchavarri jchavarri Mar 3, 2023

Choose a reason for hiding this comment

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

Tried something in 81d3339.

@jchavarri
return sourced_module from obj_map
559e885 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri jchavarri force-pushed the perf/run-parse_compilation_units-once branch from c8e81be to 559e885 Compare March 2, 2023 23:04
jchavarri added 2 commits March 3, 2023 19:50
@jchavarri
simplify to_modules handling
81d3339 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
@jchavarri
Merge branch 'main' into perf/run-parse_compilation_units-once
263a7c0 
Signed-off-by: Javier Chávarri <javier.chavarri@gmail.com>
rgrinberg
rgrinberg reviewed Mar 3, 2023
View reviewed changes
.github/workflows/bench.yml
branches:
- main
- bench/add-warm-run
- perf/run-parse_compilation_units-once
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg Mar 3, 2023

Choose a reason for hiding this comment

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

Should this be moved to a separate commit?

@rgrinberg
_
df4c4a5 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg added this to the 3.8.0 milestone Mar 3, 2023
@rgrinberg
_
fbd5aae 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Mar 3, 2023

I simplified things a little further. We don't need Obj_map at all.

@rgrinberg rgrinberg merged commit 5bdb844 into ocaml:main Mar 3, 2023
@emillon emillon mentioned this pull request Apr 18, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Apr 18, 2023
@emillon
[new release] dune (15 packages) (3.8.0~alpha1)
8f04d31 
CHANGES:

- When a rule's action is interrupted, delete any leftover directory targets.
  This is consistent with how we treat file targets. (@rgrinberg, 7564)

- Fix plugin loading with findlib. The functionality was broken in 3.7.0.
  (ocaml/dune#7556, @anmonteiro)

- Introduce a `public_headers` field on libraries. This field is like
  `install_c_headers`, but it allows to choose the extension and choose the
  paths for the installed headers. (ocaml/dune#7512, @rgrinberg)

- Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes
  ocaml/dune#1701, @rgrinberg, @anmonteiro)

- Resolve `ppx_runtime_libraries` in the target context when cross compiling
  (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro)

- Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary  (ocaml/dune#7469, fixes
  ocaml/dune#2572, @anmonteiro)

- Preliminary support for Coq compiled intefaces (`.vos` files) enabled via
  `(mode vos)` in `coq.theory` stanzas. This can be used in combination with
  `dune coq top` to obtain fast re-building of dependencies (with no checking
  of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre)

- Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD`
  (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg)

- Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro,
  @rizo)

- Find `pps` dependencies in the host context when cross-compiling,  (ocaml/dune#7410,
  fixes ocaml/dune#4156, @anmonteiro)

- Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395
  @rgrinberg, @jchavarri)

- `dune coq top` now correctly respects the project root when called from a
  subdirectory. However, absolute filenames passed to `dune coq top` are no
  longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and
  @Alizter)

- Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380,
  fixes ocaml/dune#7355, @Alizter)

- RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes
  ocaml/dune#6879, @rgrinberg)

- Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg)

- Bootstrap: remove reliance on shell. Previously, we'd use the shell to get
  the number of processors. (ocaml/dune#7274, @rgrinberg)

- Bootstrap: correctly detect the number of processors by allowing `nproc` to be
  looked up in `$PATH` (ocaml/dune#7272, @Alizter)

- Speed up file copying on macos by using `clonefile` when available
  (@rgrinberg, ocaml/dune#7210)

- Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081,
  @ivg, @richardlford)

- Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188,
  @Alizter)

- Speed up rule generation for libraries and executables with many modules
  (ocaml/dune#7187, @jchavarri)

- Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford)

- Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix
  ocaml/dune#7184, @rgrinberg)

- Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133)

- Non-user proccesses such as version control or config checking are now run
  silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter)

- Add the `--display-separate-messages` flag to separate the error messages
  produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope)

- Accept the Ordered Set Language for the `modes` field in `library` stanzas
  (ocaml/dune#6611, @anmonteiro).

- dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes
  ocaml/dune#7106, @Alizter)

- Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not
  be used as dependencies of libraries in the workspace (eg when compiling to
  bytecode and/or Javascript).  This is now fixed. (ocaml/dune#7151, @nojb)

- Allow the main module of a library with `(stdlib ...)` to depend on other
  libraries (ocaml/dune#7154, @anmonteiro).

- Bytecode executables built for JSOO are linked with `-noautolink` and no
  longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb)

- Added a new user action `(concurrent )` which is like `(progn )` but runs the
  actions concurrently. (ocaml/dune#6933, @Alizter)

- Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas
  (ocaml/dune#7139, @anmonteiro).

- Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo)

- Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416,
  @nojb)

- Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445,
  @Leonidas-from-XIV)

- Always include `opam` files in the generated `.install` file. Previously, it
  would not be included whenever `(generate_opam_files true)` was set and the
  `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg)
@emillon emillon mentioned this pull request May 23, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request May 23, 2023
@emillon
[new release] dune (15 packages) (3.8.0)
df1ab22 
CHANGES:

- Fix string quoting in the json file written by `--trace-file` (ocaml/dune#7773,
  @rleshchinskiy)

- Read `pkg-config` arguments from the `PKG_CONFIG_ARGN` environment variable
  (ocaml/dune#1492, ocaml/dune#7734, @anmonteiro)

- Correctly set `MANPATH` in `dune exec`. Previously, we would use the `bin/`
  directory of the context. (ocaml/dune#7655, @rgrinberg)

- Allow overriding the `ocaml` binary with findlib configuration (ocaml/dune#7648,
  @rgrinberg)

- merlin: ignore instrumentation settings for preprocessing. (ocaml/dune#7606, fixes
  ocaml/dune#7465, @Alizter)

- When a rule's action is interrupted, delete any leftover directory targets.
  This is consistent with how we treat file targets. (ocaml/dune#7564, @rgrinberg)

- Fix plugin loading with findlib. The functionality was broken in 3.7.0.
  (ocaml/dune#7556, @anmonteiro)

- Introduce a `public_headers` field on libraries. This field is like
  `install_c_headers`, but it allows to choose the extension and choose the
  paths for the installed headers. (ocaml/dune#7512, @rgrinberg)

- Load the host context `findlib.conf` when cross-compiling (ocaml/dune#7428, fixes
  ocaml/dune#1701, @rgrinberg, @anmonteiro)

- Add a `coqdoc_flags` field to the `coq.theory` stanza allowing the user to
  pass extra arguments to `coqdoc`. (ocaml/dune#7676, fixes ocaml/dune#7954 @Alizter)

- Resolve `ppx_runtime_libraries` in the target context when cross compiling
  (ocaml/dune#7450, fixes ocaml/dune#2794, @anmonteiro)

- Use `$PKG_CONFIG`, when set, to find the `pkg-config` binary  (ocaml/dune#7469, fixes
  ocaml/dune#2572, @anmonteiro)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will cause Dune to emit a warning which will become an error in 3.9. (ocaml/dune#7608,
  fixes ocaml/dune#7026, @Alizter)

- Preliminary support for Coq compiled intefaces (`.vos` files) enabled via
  `(mode vos)` in `coq.theory` stanzas. This can be used in combination with
  `dune coq top` to obtain fast re-building of dependencies (with no checking
  of proofs) prior to stepping into a file. (ocaml/dune#7406, @rlepigre)

- Fix dune crashing on MacOS in watch mode whenever `$PATH` contains `$PWD`
  (ocaml/dune#7441, fixes ocaml/dune#6907, @rgrinberg)

- Fix `dune install` when cross compiling (ocaml/dune#7410, fixes ocaml/dune#6191, @anmonteiro,
  @rizo)

- Find `pps` dependencies in the host context when cross-compiling,  (ocaml/dune#7410,
  fixes ocaml/dune#4156, @anmonteiro)

- Dune in watch mode no longer builds concurrent rules in serial (ocaml/dune#7395
  @rgrinberg, @jchavarri)

- Dune can now detect Coq theories from outside the workspace. This allows for
  composition with installed theories (not necessarily installed with Dune).
  (ocaml/dune#7047, @Alizter, @ejgallego)

- `dune coq top` now correctly respects the project root when called from a
  subdirectory. However, absolute filenames passed to `dune coq top` are no
  longer supported (due to being buggy) (ocaml/dune#7357, fixes ocaml/dune#7344, @rlepigre and
  @Alizter)

- Added a `--no-build` option to `dune coq top` for avoiding rebuilds (ocaml/dune#7380,
  fixes ocaml/dune#7355, @Alizter)

- RPC: Ignore SIGPIPE when clients suddenly disconnect (ocaml/dune#7299, ocaml/dune#7319, fixes
  ocaml/dune#6879, @rgrinberg)

- Always clean up the UI on exit. (ocaml/dune#7271, fixes ocaml/dune#7142 @rgrinberg)

- Bootstrap: remove reliance on shell. Previously, we'd use the shell to get
  the number of processors. (ocaml/dune#7274, @rgrinberg)

- Bootstrap: correctly detect the number of processors by allowing `nproc` to be
  looked up in `$PATH` (ocaml/dune#7272, @Alizter)

- Speed up file copying on macos by using `clonefile` when available
  (@rgrinberg, ocaml/dune#7210)

- Adds support for loading plugins in toplevels (ocaml/dune#6082, fixes ocaml/dune#6081,
  @ivg, @richardlford)

- Support commands that output 8-bit and 24-bit colors in the terminal (ocaml/dune#7188,
  @Alizter)

- Speed up rule generation for libraries and executables with many modules
  (ocaml/dune#7187, @jchavarri)

- Add `--watch-exclusions` to Dune build options (ocaml/dune#7216, @jonahbeckford)

- Do not re-render UI on every frame if the UI doesn't change (ocaml/dune#7186, fix
  ocaml/dune#7184, @rgrinberg)

- Make coq_db creation in scope lazy (@ejgallego, ocaml/dune#7133)

- Non-user proccesses such as version control or config checking are now run
  silently. (ocaml/dune#6994, fixes ocaml/dune#4066, @Alizter)

- Add the `--display-separate-messages` flag to separate the error messages
  produced by commands with a blank line. (ocaml/dune#6823, fixes ocaml/dune#6158, @esope)

- Accept the Ordered Set Language for the `modes` field in `library` stanzas
  (ocaml/dune#6611, @anmonteiro).

- dune install now respects --display quiet mode (ocaml/dune#7116, fixes ocaml/dune#4573, fixes
  ocaml/dune#7106, @Alizter)

- Stub shared libraries (dllXXX_stubs.so) in Dune-installed libraries could not
  be used as dependencies of libraries in the workspace (eg when compiling to
  bytecode and/or Javascript).  This is now fixed. (ocaml/dune#7151, @nojb)

- Allow the main module of a library with `(stdlib ...)` to depend on other
  libraries (ocaml/dune#7154, @anmonteiro).

- Bytecode executables built for JSOO are linked with `-noautolink` and no
  longer depend on the shared stubs of their dependent libraries (ocaml/dune#7156, @nojb)

- Added a new user action `(concurrent )` which is like `(progn )` but runs the
  actions concurrently. (ocaml/dune#6933, @Alizter)

- Allow `(stdlib ...)` to be used with `(wrapped false)` in library stanzas
  (ocaml/dune#7139, @anmonteiro).

- Allow parallel execution of inline tests partitions (ocaml/dune#7012, @hhugo)

- Support `(link_flags ...)` in `(cinaps ...)` stanza. (ocaml/dune#7423, fixes ocaml/dune#7416,
  @nojb)

- Allow `(package ...)` in any position within `(rule ...)` stanza (ocaml/dune#7445,
  @Leonidas-from-XIV)

- Always include `opam` files in the generated `.install` file. Previously, it
  would not be included whenever `(generate_opam_files true)` was set and the
  `.install` file wasn't yet generated. (ocaml/dune#7547, @rgrinberg)

- Fix regression where Merlin was unable to handle filenames with uppercase
  letters under Windows. (ocaml/dune#7577, @nojb)

- On nix+macos, pass `-f` to the codesign hook to avoid errors when the binary
  is already signed (ocaml/dune#7183, fixes ocaml/dune#6265, @greedy)

- Fix bug where RPC clients built with dune-rpc-lwt would crash when closing
  their connection to the server (ocaml/dune#7581, @gridbugs)

- Introduce mdx stanza 0.4 requiring mdx >= 2.3.0 which updates the default
  list of files to include `*.mld` files (ocaml/dune#7582, @Leonidas-from-XIV)

- Fix RPC server on Windows (used for OCaml-LSP). (ocaml/dune#7666, @nojb)

- Coq language versions less 0.8 are deprecated, and will be removed
  in an upcoming Dune version. All users are required to migrate to
  `(coq lang 0.8)` which provides the right semantics for theories
  that have been globally installed, such as those coming from opam
  (@ejgallego, @Alizter)

- Bump minimum version of the dune language for the melange syntax extension
  from 3.7 to 3.8 (ocaml/dune#7665, @jchavarri)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

Assignees

@jchavarri jchavarri

Labels

None yet

Projects

None yet

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

2 participants

@jchavarri @rgrinberg