-
Notifications
You must be signed in to change notification settings - Fork 19
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
optimize: speed up stat gen by factor x15 #167
Conversation
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 left some comments about things to think about.
Self::generate_partial_stats(&comb, mgs, hlls, null_cnts); | ||
Ok(local_stats) | ||
} | ||
Err(_) => todo!(), // TODO(Alexis): Could not satisfy the type checker otherwise, but never happens! |
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.
If it never happens you can panic!()
here with a description in the panic for why it never happens
@@ -410,7 +414,7 @@ impl DatafusionDBMS { | |||
&mut self, | |||
job_kit_config: &JobKitConfig, | |||
) -> anyhow::Result<DataFusionBaseTableStats> { |
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.
If you're modifying get_job_stats()
you should also modify get_tpch_stats()
in the same way. In the future we can think about creating a trait called BenchmarkKit
and making a generic function that works with any object of that trait. However, for now unfortunately there's some copied code here.
Made the stat generation faster using Rayon's thread pool.
Various improvements, such as;
This takes 30s for JOB 1D stats on my computer, vs 7:30min before.
Postgres takes 1:30min loading, and 22s for the stat gen. So we beat it, depending on how we view it.
On "real" datacenter hardware (i.e. 512 cores), we would crush it, we'll test that soon.
Finally coming together :-)