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

Add monorepo benchmark #7202

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Add monorepo benchmark #7202

merged 2 commits into from
Mar 16, 2023

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Mar 1, 2023

This adds a current-bench benchmark that measures the time it takes to build a large monorepo composed from opam packages.

@rgrinberg
Copy link
Member

Would it be possible to move monorepo-bench to bench/monorepo?

(modules bench)
(libraries unix))

(library
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this mega library? It's not enough to build everything individually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The monorepo is made up of many packages and some of them contain libraries that don't build. I've made a list of the libraries that don't build and used them to construct the list of libraries in this file.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean don't build? As in fail to build, or aren't being requested to build at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean that they fail to build in a monorepo setting.

Copy link
Member

Choose a reason for hiding this comment

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

I see. So building this mega library is supposed to fail as well then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the list of libraries in this file are just the ones that build in a monorepo setting.

$(RUNNER): dune bench.ml
dune build $@ --release

bench: $(RUNNER)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this depend on $(DUNE_TO_BENCHMARK) as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. $(DUNE_TO_BENCHMARK) isn't a target of this makefile; it's assumed that it's already been built when make bench is run.

dune Outdated
@@ -0,0 +1 @@
(data_only_dirs monorepo-bench)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works since we have a top level dune-file instead. In any case, I'd like to see this under bench/ if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah you're right, even with this file when I run dune build it attempts to build the project in monorepo-bench. I'll move the monorepo benchmark inside the "bench" directory but I still need to find a way to prevent the monorepo bench project being seen by dune. My intention is that it should only ever be built inside a docker container when running the benchmark.

Copy link
Member

Choose a reason for hiding this comment

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

You could exclude it with (dirs ..).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up moving the monorepo-bench dir to bench/monorepo and adding (dirs :standard \ monorepo) to bench/dune. I don't understand why (dirs ..) works. Did you mean that putting (dirs ..) in a dune file would exclude that file's directory from the build? Why is it necessary to refer to the parent directory?

@gridbugs
Copy link
Collaborator Author

gridbugs commented Mar 6, 2023

I've moved the monorepo benchmark to bench/monorepo and fixed the dune files so it's excluded from normal builds. As an experiment I tried getting rid of the (library ...) stanza with the long list of dune files, but afterwards running dune build doesn't build any of the packages in duniverse. Is there different command I should be using to exercise dune in this situation? It's probably moot though because if I was to, say, add a library dependency for all the libraries in all the packages included in the monorepo, there would be some that don't build in a monorepo setting. I maintain a list of libraries that don't build in such a setting and have excluded them from the list of library dependencies of the monorepo library.

@rgrinberg
Copy link
Member

but afterwards running dune build doesn't build any of the packages in duniverse. Is there different command I should be using to exercise dune in this situation?

Which command did you try?

@gridbugs
Copy link
Collaborator Author

gridbugs commented Mar 8, 2023

I tried running dune build and dune build @all but neither of them built targets inside "duniverse". If I remove (vendored_dirs *) from that directory then both of those commands bulid targets inside it.

@rgrinberg
Copy link
Member

If I remove (vendored_dirs *) from that directory then both of those commands bulid targets inside it.

How annoying. I really regret adding all this special behavior to vendored directories. We can either:

  1. Remove vendored_dirs and your library stanza
  2. Keep things as is and leave a note as to why.

Your choice.

@gridbugs
Copy link
Collaborator Author

gridbugs commented Mar 8, 2023

I'm going to elect to keep things as they are (I'll update this PR with a note explaining why) as some libraries don't build in a monorepo setting, so without the specific list of libraries to build we would have to remove packages so that every library in every remaining package builds in a monorepo setting.

bench/monorepo/bench.ml Outdated Show resolved Hide resolved
@gridbugs
Copy link
Collaborator Author

After discussing this with @snowleopard I'm moving most of this benchmark to a separate repo (currently https://github.com/gridbugs/ocaml-monorepo-benchmark/tree/main/benchmark but the plan is to move it to https://github.com/ocaml-dune). I'll keep the bench.Dockerfile and benchmark runner tool in this PR and update the dockerfile to download the rest of the benchmark from its external location. The reasoning behind this is to avoid adding the ~1mb opam-monorepo lockfile to dune, consolidating the benchmarks with the tool that generates the benchmarks (also in the ocmal-monorepo-benchmark repo) and so that the monorepo can be used for non-dune purposes in the future.

@snowleopard
Copy link
Collaborator

snowleopard commented Mar 10, 2023

To add one more reason: I think it's worth planning to have more than one monorepo benchmark in future, and pushing all of them to the Dune repo doesn't seem right.

@gridbugs gridbugs force-pushed the monorepo-bench branch 4 times, most recently from 6aa01af to 9a1186b Compare March 13, 2023 07:45
@gridbugs
Copy link
Collaborator Author

I've moved all the files specific to the content of the benchmark to https://github.com/ocaml-dune/ocaml-monorepo-benchmark and made a tag of that repo 2023-03-13.0 which gets downloaded when building the docker image for the monorepo benchmark.

@gridbugs
Copy link
Collaborator Author

@rgrinberg regarding the dune file listing all the library dependencies, I've moved it to the ocaml-monorepo-benchmark repo (here https://github.com/ocaml-dune/ocaml-monorepo-benchmark/blob/main/benchmark/dune). I've also added to that repo a description of why the file is necessary, in https://github.com/ocaml-dune/ocaml-monorepo-benchmark/blob/main/benchmark/README.md.

Are there any other changes needed before we merge this?

This adds a current-bench benchmark that measures the time it takes to
build a large monorepo composed from opam packages.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@rgrinberg rgrinberg merged commit c0c0acb into ocaml:main Mar 16, 2023
@Alizter
Copy link
Collaborator

Alizter commented Mar 20, 2023

@gridbugs Could you add a note in the benchmark section of hacking.rst how to use this?

@gridbugs
Copy link
Collaborator Author

@gridbugs Could you add a note in the benchmark section of hacking.rst how to use this?

Yep, here: #7361

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.

5 participants