-
Notifications
You must be signed in to change notification settings - Fork 14
Report host stats on TPCH benchmarks #131
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
Conversation
jayshrivastava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
NGA-TRAN
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Th ePR looks good. Some questions regarding cores & threads
| // Print machine information | ||
| println!("os: {}", std::env::consts::OS); | ||
| println!("arch: {}", std::env::consts::ARCH); | ||
| println!("cpu cores: {}", get_available_parallelism()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the number of cores per worker or total of cpu cores of all workers?
Is there any constraints between this and number of threads per worker? Should number of threads per worker < CPU cores per worker? Is there any enforcement or unexpected behavior if it is not enforced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the number of cores per worker or total of cpu cores of all workers?
Total CPU cores available in the machine that runs the benchmarks
Is there any constraints between this and number of threads per worker?
Only the ones specified by the people running the commands, it's possible to do --threads 1000 in a 2 core CPU machine. It does not make much sense to do that though...
Should number of threads per worker < CPU cores per worker?
Assuming get_available_parallelism() returns the number of CPU cores in the machine, I think so yes. As these are just benchmarks meant to be run by us, I think it's fine to not do any enforcement in the code, hopefully people running the benchmarks know what they are doing if the do --threads 1000 in a 2 core CPU machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree people who run the benchmark should know what we are doing. Maybe we just help remind them that. How about we add a brief message saying number of threads should be < number of cores and number of partitions should be < number of threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a doc comment in the CLI
…t/report-host-stats-on-benchmark
# Conflicts: # benchmarks/src/tpch/run.rs
Reports some relevant data about the environment in which the TPCH benchmarks are running: