Skip to content

Conversation

@edwintorok
Copy link
Contributor

@edwintorok edwintorok commented Jul 22, 2024

This reduces uncached full build times by about 15%, and test times by about 10%.

It does this by:

  • avoiding to build bytecode for libraries (though libraries with public_name are still built as bytecode due to a dune bug modes not respected in combination with library with public_name ocaml/dune#3467). This could potentially be further reduced by some of the .opam cleanup/unification that I'm working on.
  • using a .mli for large files like server.ml (it only exports a single function, this alone cuts build times by about 15%)
  • avoiding to preprocess files that don't use PPX features
  • rearranging modules and removing unneeded dependencies such that some of the tests could run earlier, in parallel with the server.ml/client.ml build

Interestingly the koji build times are almost exactly the same, so this only seems to help with dev builds (i.e. dune build and dune build @check), not release buids (release builds introduce additional dependencies and reduce parallelism due to also enabling cross-module inlining).

Some of these commits have been around since 2023, although at the time there were some inexplicable test failures in forkexecd that only happened on the CI (likely exposing latent bugs): #4939
In this PR I've only cherry-picked the build changes, and didn't cherry-pick the forkexecd changes.


hyperfine --min-runs 3 'dune clean; dune build --cache=disabled' 'cd ../scm-prev; dune clean; dune build --cache=disabled'
Benchmark 1: dune clean; dune build --cache=disabled
  Time (mean ± σ):     79.936 s ±  0.666 s    [User: 343.353 s, System: 116.654 s]
  Range (min … max):   79.373 s … 80.671 s    3 runs

Benchmark 2: cd ../scm-prev; dune clean; dune build --cache=disabled
  Time (mean ± σ):     91.555 s ±  0.613 s    [User: 355.560 s, System: 118.064 s]
  Range (min … max):   91.083 s … 92.248 s    3 runs

Summary
  dune clean; dune build --cache=disabled ran
    1.15 ± 0.01 times faster than cd ../scm-prev; dune clean; dune build --cache=disabled
hyperfine --min-runs 2 'dune clean; dune runtest --cache=disabled' 'cd ../scm-prev; dune clean; dune runtest --cache=disabled'
Benchmark 1: dune clean; dune runtest --cache=disabled
  Time (mean ± σ):     103.491 s ±  1.596 s    [User: 374.464 s, System: 125.957 s]
  Range (min … max):   102.363 s … 104.620 s    2 runs

Benchmark 2: cd ../scm-prev; dune clean; dune runtest --cache=disabled
  Time (mean ± σ):     114.158 s ±  2.980 s    [User: 380.638 s, System: 134.558 s]
  Range (min … max):   112.051 s … 116.266 s    2 runs

Summary
  dune clean; dune runtest --cache=disabled ran
    1.10 ± 0.03 times faster than cd ../scm-prev; dune clean; dune runtest --cache=disabled

@edwintorok edwintorok force-pushed the private/edvint/maintenance2 branch from 0927fb7 to 7889467 Compare July 22, 2024 17:19
@edwintorok edwintorok force-pushed the private/edvint/maintenance2 branch from 7889467 to f4c6ecc Compare July 23, 2024 09:32
@edwintorok
Copy link
Contributor Author

Cherry picked a few more improvements from the old branch, and rearranged dependencies.

dune runtest: 97s, compared to the initial 114s, ~15% improvement
dune build: 67s, compared to the initial 91s, ~25% improvement
koji build time: 276s, compared to the initial 325s, ~15% improvement

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

If this builds hard to imagine it's not correct.

Some features of Dune are only available when a new language version is used
(e.g. 'package' for 'library' stanzas would require bumping this to 2.8).

Defaults also change, e.g. 3.0+ enables `executables_implicit_empty_intf`
which can be beneficial for finding dead code in executables.

But more importantly Dune versions <3.7 have a binary corruption bug with
executable promotion that got fixed here:
ocaml/dune@f0c708c

Require dune >= 3.7.

The version bumps also comes with many more unused warnings enabled by default,
turn these back into warnings and do not fail the build.
(Once they are fixed we can remove the -warn-error list)

No functional change.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This is the version we currently use in xs-opam.
Newer dune version may also come with more warnings enabled by default.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
No functional change.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Only build 'best' (which may be bytecode if native is not available).

Note that this does not prevent the use of 'dune utop': it will build bytecode
libraries as needed, they are just not built by default.
(And since they are internal libraries they wouldn't get installed anyway)

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Speeds up the build, together with the previous commit.

Now we no longer need to build bytecode version of server.ml.

A similar approach might be useful for db_actions and client,
but those would have to be generated automatically.

```
hyperfine --min-runs 3 'dune clean; dune build --cache=disabled' 'cd ../scm-prev; dune clean; dune build --cache=disabled'
Benchmark 1: dune clean; dune build --cache=disabled
  Time (mean ± σ):     79.936 s ±  0.666 s    [User: 343.353 s, System: 116.654 s]
  Range (min … max):   79.373 s … 80.671 s    3 runs

Benchmark 2: cd ../scm-prev; dune clean; dune build --cache=disabled
  Time (mean ± σ):     91.555 s ±  0.613 s    [User: 355.560 s, System: 118.064 s]
  Range (min … max):   91.083 s … 92.248 s    3 runs

Summary
  dune clean; dune build --cache=disabled ran
    1.15 ± 0.01 times faster than cd ../scm-prev; dune clean; dune build --cache=disabled
```

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Reduces the amount of work the build has to do if we don't need to preprocess
everything, but only the few modules that actually using [@@deriving].

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This signature is completely unused.
We could instead generate a client.mli, but that is more complicated,
currently the client.mli it'd generate wouldn't be polymorphic enough.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Server.ml takes a while to compile, but most unit tests don't actually need it.
Reorganize Api_server into Api_server+Api_server_common, where the latter
suffices for unit tests.

'dune runtest' times are improved:
```
hyperfine --min-runs 2 'dune clean; dune runtest --cache=disabled' 'cd ../scm-prev; dune clean; dune runtest --cache=disabled'
Benchmark 1: dune clean; dune runtest --cache=disabled
  Time (mean ± σ):     103.491 s ±  1.596 s    [User: 374.464 s, System: 125.957 s]
  Range (min … max):   102.363 s … 104.620 s    2 runs

Benchmark 2: cd ../scm-prev; dune clean; dune runtest --cache=disabled
  Time (mean ± σ):     114.158 s ±  2.980 s    [User: 380.638 s, System: 134.558 s]
  Range (min … max):   112.051 s … 116.266 s    2 runs

Summary
  dune clean; dune runtest --cache=disabled ran
    1.10 ± 0.03 times faster than cd ../scm-prev; dune clean; dune runtest --cache=disabled
```

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
`sexpr` is now fully thread safe without having to use locks,
doesn't need to depend on threadext.

`gen_api_main` can use the external `uuidm` module directly,
without waiting for internal one to be built.

`dune-build-info` is only needed by xapi_version.

`xapi-stdext-unix` is not needed in `xapi-idl`

The sexplib ppx runtime also doesn't need to be linked in some libraries that do not use it anymore,
and where it is used it'll be automatically linked.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Xapi_version depends on Build_info which can change on every commit.
It is better to remove it from the dependencies of gen_api_main,
especially that gen_api_main is on the critical path for discovering more
dependencies.

The 'xapi_user_agent' constant got moved to Xapi_version.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
It'll be slower, but it can run a lot earlier in the build process.
Compiling the datamodels takes time, but compiling them for bytecode is faster.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Introduce a _minimal library, so we can start compiling server.ml earlier.

Build time reduced from 80s to:
```
Benchmark 1: dune clean; dune build --cache=disabled
  Time (mean ± σ):     67.081 s ±  0.190 s    [User: 326.847 s, System: 103.668 s
  Range (min … max):   66.946 s … 67.215 s    2 runs
```

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Remove most sleeps, and reduce test duration from 5 seconds to 1.
(If we do want to run a performance test we can increase these again)

Run just a basic test for 0.1 seconds instead of a performance test for 5s by
default.
(can still be tweaked by overriding SECS)

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Instead of every 5s. Speeds up testing, and may speed up startup somewhat.
And a connection try once every 0.5s won't create a lot of load on the system.
(If needed we could implement some form of exponential backoff here).

```
  Benchmark 1: dune clean; dune runtest --cache=disabled
  Time (mean ± σ):     97.642 s ±  0.933 s    [User: 354.132 s, System: 113.436 s]
  Range (min … max):   96.982 s … 98.302 s    2 runsi
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
@edwintorok edwintorok force-pushed the private/edvint/maintenance2 branch from f4c6ecc to 7530d5e Compare July 23, 2024 22:38
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
@edwintorok edwintorok merged commit bc549cd into xapi-project:master Jul 24, 2024
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.

4 participants