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

Comparison benchmarks and comparison in Readme #113

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jdonszelmann
Copy link
Collaborator

@jdonszelmann jdonszelmann commented Jun 9, 2023

Based on #112

@github-actions github-actions bot temporarily deployed to commit June 9, 2023 08:05 Inactive
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

@github-actions github-actions bot temporarily deployed to pull request June 9, 2023 08:08 Inactive
@github-actions github-actions bot temporarily deployed to commit June 9, 2023 08:08 Inactive
@jdonszelmann jdonszelmann changed the title Benchmarks vs others Comparison benchmarks and comparioson in Readme Jun 9, 2023
let mut vd = Vec::with_capacity(CAP);

b.iter(|| {
for i in 0..ITER {
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't we push until CAP and then start doing remove & push, to have the number of elements stay on CAP

Copy link
Owner

Choose a reason for hiding this comment

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

remove from front push on back specifically as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, I guess that makes sure that the first iteration is not an outlier

README.md Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request June 9, 2023 08:12 Inactive
@github-actions github-actions bot temporarily deployed to commit June 9, 2023 08:12 Inactive
@NULLx76 NULLx76 changed the title Comparison benchmarks and comparioson in Readme Comparison benchmarks and comparison in Readme Jun 9, 2023
Co-authored-by: Victor <[email protected]>
@github-actions github-actions bot temporarily deployed to commit June 9, 2023 08:14 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 9, 2023 08:14 Inactive
@github-actions github-actions bot temporarily deployed to commit June 9, 2023 08:17 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 9, 2023 08:17 Inactive
let _ = rb.push(i);
black_box(());
}
for i in 0..ITER {
Copy link
Owner

@NULLx76 NULLx76 Jun 9, 2023

Choose a reason for hiding this comment

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

Doing this more than CAP amount of time is just a no-op right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sort of, the buffer has to reject the items all the rest of the times which might take time just as our dropping of old items does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry that's for the heapless version. Here it will drop old items ITER-CAP times

Copy link
Owner

Choose a reason for hiding this comment

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

I'm just kinda confused why we are testing different things for different structures, shouldn't we test all under the use case of:

A buffer which can contain at max CAP items, removing old entries as new ones get pushed. Clearing out the buffer at the end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no other datastructure has that behavior. They're different datastructures, we cannot test them under the same circumstances

Copy link
Owner

@NULLx76 NULLx76 Jun 9, 2023

Choose a reason for hiding this comment

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

Then the comparison is meaningless imo, it should be about using similar datastructures for the same purpose. I think we can emulate ringbuffer behaviour on most if not all of these datastructures

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 disagree. No benchmark can completely fairly compare these datastructures, but it does give a good overview of differences in order of magnitude. Of course a channel is not a ringbuffer, but pushing 2^14 items to a channel takes about 14 times longer than to a ringbuffer which deals with removing old items. Simply pushing 2^14 items to a vecdeque that has to deal with growing is about 3x slower than pushing to a ringbuffer. Pushing to a heapless deque which drops items on insert is a few percent slower than pushing to a ringbuffer which deletes old items.

None of them are the same datastructure, but it does show the order of magnitude of differences you can expect when using these datastructures. Each datastructure is good at different things, but this is their normal performance when you use them as a queue.

In fact, I don't really want to modify these datastructures to function identically to ringbuffer. Then we don't measure how these datastructures normally perform as a queue for the purpose they were designed for. Instead we measure how well we can turn the datastructure into a ringbuffer.

Copy link
Owner

Choose a reason for hiding this comment

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

As long as we add a disclaimer/description of the benchmarks in the readme this is fine then I think

@github-actions github-actions bot temporarily deployed to commit June 10, 2023 11:09 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 10, 2023 11:09 Inactive
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