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 note about --cpu-list in README #333

Conversation

moazzammoriani
Copy link
Contributor

Fixes #332.

Copy link
Contributor

@Sudha247 Sudha247 left a comment

Choose a reason for hiding this comment

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

Thanks! This is indeed useful to mention. Just a minor correction suggested below.

README.md Outdated
assumed that your machine has at least 5 CPU cores. You may want to adjust the
`--cpu-list` parameter based on whatever number of CPU cores your machine has
or if you don't know how many cores it has then consider setting the parameter
to `--cpu-list 1` to use only one CPU core.
Copy link
Contributor

Choose a reason for hiding this comment

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

--cpu-list 5 actually runs the benchmark on only one core, which is core number 5. Making it --cpu-list 1 would run it on core number 1 instead. This line sort of makes it sound like the former case runs the benchmarks on more than one core, which it doesn't. Would be nice to clarify that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your feedback is much appreciated. I have made the some edits that take your points in consideration feel free to take a look at them whenever you can (:

@moazzammoriani moazzammoriani force-pushed the Add_note_on_cpu-list_in_README branch 5 times, most recently from 274f440 to 52372fa Compare April 12, 2022 18:30
@moazzammoriani
Copy link
Contributor Author

Seems like the loadavg wouldn't go down so the CI build was killed

@@ -212,6 +212,18 @@ The scripts for latencies are present in the `pausetimes/` directory. The
`pausetimes_trunk` Bash script obtains the latencies for stock OCaml and the
`pausetimes_multicore` Bash script for Multicore OCaml.

**NOTE**: You may need to modify the `--cpu-list` parameter in the config file
Copy link
Contributor

Choose a reason for hiding this comment

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

It is useful to add a note about --cpu-list to the README, but, this is not sufficient. One needs to also disable hyperthreading, setup isolcpus, remove RCU callbacks etc. as mentioned in the Notes on hardware and OS settings for Linux benchmarking section at https://github.com/ocaml-bench/ocaml_bench_scripts.

assumed that your machine has a fifth core to use for the build. If your build
fails then a possible reason might be that your machine doesn't have a fifth
CPU core. In such a situation you might want to modify the parameter to use
any number of cores less than or equal to the number of cores your machine
Copy link
Contributor

Choose a reason for hiding this comment

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

One must use only the isolated CPUs.

any number of cores less than or equal to the number of cores your machine
has. If you do not know the number of cores your machine has then your safest
option is to modify to the parameter to `--cpu-list 1` to simply use the first
core.
Copy link
Contributor

Choose a reason for hiding this comment

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

One needs to know the machine on which the cores are running. Usually, the first few cores are non-isolated CPUs, and are used by the OS.

Copy link
Contributor Author

@moazzammoriani moazzammoriani Apr 18, 2022

Choose a reason for hiding this comment

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

This reasoning stems from the fact that it was usually recommended to set --cpu-list to 1 by you or @Sudha247. Inference filled the rest of the gaps I suppose. Would you say that I should remove this suggestion of setting it to 1 altogether and instead suggest to set --cpu-list to be some non-isolated CPU?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just setting the --cpu-list is not sufficient to run the parallel benchmarks. A detailed information on the setup is required, and will be added as part of HACKING.md.

Copy link
Contributor Author

@moazzammoriani moazzammoriani Apr 18, 2022

Choose a reason for hiding this comment

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

Then perhaps this note should either point to HACKING.md or should be removed altogether I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

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

A note on --cpu-list will be added to HACKING.md.

@shakthimaan shakthimaan closed this May 9, 2022
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.

Add note about --cpu-list parameter in README
3 participants