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

Improve benchmarks #1034

Merged
merged 4 commits into from
Nov 7, 2023
Merged

Improve benchmarks #1034

merged 4 commits into from
Nov 7, 2023

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented Oct 31, 2023

@Julow Julow added the no changelog This pull request does not need a changelog entry label Oct 31, 2023
Makefile Show resolved Hide resolved
doc/driver.mld Outdated
Comment on lines 303 to 311
"base";
"core_kernel";
"bin_prot";
"sexplib";
"sexplib0";
"base_quickcheck";
"ppx_sexp_conv";
"ppx_hash";
"core";
Copy link
Collaborator

Choose a reason for hiding this comment

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

How were the previous "extra deps" chosen? If the reason was valid, why not keeping them, adding "core"?

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 think the intent of the previous list was to have a world that is as large as core but that is not core, because it failed to build at some point. I might be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

No, it was that core had some dependencies and some were vaguely interesting, so might as well build them too. Particularly anything that core/core_kernel might have linked to (so sexplib, bin_prot, etc).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I added them back. Though I did not add more as that would make the list very large without adding interesting test cases.

Copy link
Member

Choose a reason for hiding this comment

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

My thinking was mainly that certain modules take vastly longer than others, so including a new library either means it's very quick (and not interesting) or that some modules now take ages, which is very interesting, and when you fix it, becomes a useful test case :-)

doc/driver.mld Outdated Show resolved Hide resolved
The previous 'longest' metric was not useful as it was a duplicate of
the other running time metrics that output a 'max' value.

Make this metric more useful by averaging the 5 longest runs. This
correspond roughly to the top 1%.
Julow added 2 commits November 2, 2023 12:45
Currently, the benchmarks are running the driver with no extra work.
As the driver reports errors with promotion and not with an exit status,
the driver could fail to generate `driver-benchmarks.json`, in which
case Dune would complain about the missing target and would not show the
diff containing the error.
@Julow Julow force-pushed the driver-bench-longest branch from 211442f to 2f59a5d Compare November 2, 2023 11:45
@jonludlam
Copy link
Member

OK, green tick. In it goes.

@jonludlam jonludlam merged commit dfefbcc into ocaml:master Nov 7, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants