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

bench: add GC stats to bench #8063

Merged

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jun 27, 2023

Problem

We want to have a convenient way of tracking memory gains in PRs. There is no way to do this currently apart from measuring Dune's perf by yourself.

Solution

We start collecting Gc.stats in each of our build runs in the bench. In order to do so we introduce a Metrics module which is sufficiently polymorphic to support an unzipping operation which is useful when serialising to json. The mli of the Metrics module has some detailed documentation about the metrics that we collect about a process.

During the build runs, we garbage collect and compacitify before observing the statistics.

This PR changes the names of the benchmark jobs to be more uniform (use title case). This has the side effect of discontinuing previous runs, although the old data is still available in the website. Due to this I have bumped the version number of the bench.

We also enable sandboxing in the bench.

@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 6 times, most recently from 3838e2f to 26d4c63 Compare June 27, 2023 17:57
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 27, 2023

I'm now testing this with #8052 to see any memory change.

Edit: As expected, there is a decrease of 200 in the minor word count.

@Alizter
Copy link
Collaborator Author

Alizter commented Jun 27, 2023

Now I will repush the bench commit and then push #8035.

Edit: No observable change in this one.

@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 4 times, most recently from 708319f to 9f9d24d Compare June 27, 2023 18:56
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 27, 2023

This time I pushed a dummy commit allocating a pointless hashtable in a hotpath to see if it would be picked up and it appears to have done.

image

@Alizter Alizter marked this pull request as ready for review June 27, 2023 19:07
@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch from 9f9d24d to 44b8993 Compare June 27, 2023 19:07
@Alizter Alizter requested a review from rgrinberg June 27, 2023 19:09
@rgrinberg rgrinberg requested a review from gridbugs June 28, 2023 07:17
bench/bench.ml Outdated
let+ times =
Process.run_with_times dune ~display:Quiet ~stdin_from ~stdout_to ~stderr_to
[ "build"; "@install"; "--release" ]
in
times.elapsed_time
(* Run GC and compactify before querying GC stats *)
Gc.full_major ();
Copy link
Member

Choose a reason for hiding this comment

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

You're handling the GC in the wrong process here. It's the dune command above that should spit out its GC stats.

To do this, you could add some --dump-gc-stats-end=FILE argument that would add an at_exit hook to dump this info.

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've added that option in #8072 (which is currently also included here). The data I'm getting now makes much more sense. I'll try to test out your PRs again.

@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 4 times, most recently from cd555ee to cbb92ab Compare June 28, 2023 14:11
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 28, 2023

OK this time testing #8052 again.

cc @rgrinberg

So the only observable changes are:

Major collections

image

On clean build went from 30 to 37 and on null build decreased from 23 to 22.

Heap words

image

On null build went from 2.039e+7 to 1.792e+7 but nothing changed on clean.

Free words

image

On null build free words appear to have halved from 5.126e+6 to 2.687e+6.

Conclusion

Overall, I'm not really sure if I understand what this change does to the memory.

@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 3 times, most recently from b2c621c to 79f702d Compare June 28, 2023 14:31
@Alizter
Copy link
Collaborator Author

Alizter commented Jun 28, 2023

This time trying #8035.

No changes observed really.

@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 2 times, most recently from 33c776c to 9b6f4c9 Compare June 28, 2023 16:46
bench/bench.ml Outdated Show resolved Hide resolved
bench/bench.ml Outdated Show resolved Hide resolved
@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 4 times, most recently from d0ff5e9 to 2ff8805 Compare July 5, 2023 18:14
bench/bench.ml Outdated Show resolved Hide resolved
bench/bench.ml Outdated Show resolved Hide resolved
@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch 4 times, most recently from 8774c2e to fe62fbe Compare July 8, 2023 12:36
Signed-off-by: Ali Caglayan <[email protected]>
@Alizter Alizter force-pushed the ps/branch/bench__add_gc_stats_to_bench branch from fe62fbe to 55730a2 Compare July 8, 2023 13:29
@rgrinberg
Copy link
Member

Awesome. Great work.

@rgrinberg rgrinberg merged commit 615a649 into ocaml:main Jul 8, 2023
@Alizter Alizter deleted the ps/branch/bench__add_gc_stats_to_bench branch July 8, 2023 13:34
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.

3 participants