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
Divan extra benchmarks #12025
Divan extra benchmarks #12025
Conversation
I think it's a good idea to add more benchmarks. Some of the pipeline benchmarks seem quite random, though. For example, the |
Yee the |
2f37e66
to
8996dce
Compare
After fixing conflicts this seems pretty straightforward to get into a shape so we can land those additions and iterate on our benchmarks, right? |
Yes, I have just been busy this last week will try to fix it today. :) |
This PR is ready for review. |
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 think we don't have to be perfect here yet and can observe and improve our benchmarks so we really draw some conclusions about future design decisions.
Thanks for moving us in the right direction @FilipAndersson245
load_standard_library(&mut engine).unwrap(); | ||
let commands = Spanned { | ||
span: Span::unknown(), | ||
item: format!("(1..{}) | each {{|_| sleep 50ns }} | ignore", n).to_string(), |
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.
Does using ignore
here have a relevant impact?
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 am not sure, the problem is to get rid of the output that is displayed so it doesn't clutter the screen, if there is another way to do it I'm all for it.
load_standard_library(&mut engine).unwrap(); | ||
let commands = Spanned { | ||
span: Span::unknown(), | ||
item: format!("(1..{}) | par-each -t 2 {{|_| sleep 50ns }} | ignore", n).to_string(), |
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.
shrugs at the limited number of core's on github workers
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.
Do you think this should be removed or changed?, even if cores are low just allowing multiple commands to be run parallel allow some level of parallelism even if its run on 1 thread machine. I initially added this test as I saw 1thread par-each
outperform the standard each
for low sample counts.
Description
This PR builds on #12000 and adds a few more benchmarks.
one to benchmark how long it takes to eval std, and a few different pipeline commands.
User-Facing Changes
Tests + Formatting
After Submitting