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

Re-introduce safe consolidation #481

Merged

Conversation

frankmcsherry
Copy link
Member

Consolidation previously used an unsafe implementation because it improved performance over slice::split_at_mut. This no longer appears to be the case, and also unsafe code is bad and probably doesn't work.

At the same time, the current consolidation also appears to be almost 2x as slow as a version from @antiguru that copies from one container to another, and perhaps the in-place consolidation results in loop-carried dependencies or something like that. There is certainly some more to look into here, especially as consolidation is such a common event.

@frankmcsherry frankmcsherry changed the title Re-introduce safe implementation Re-introduce safe consolidation May 2, 2024
Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

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

Looks great!

@frankmcsherry
Copy link
Member Author

This is substantially faster than master on large consolidation benchmarks, and seems comparable to master or a little better on examples/bfs benchmarks. The improvement is within measurement error, but has been consistently on the side of "improvement".

@frankmcsherry frankmcsherry merged commit de6a353 into TimelyDataflow:master May 6, 2024
7 checks passed
@frankmcsherry frankmcsherry deleted the safe_consolidation branch May 6, 2024 13:40
This was referenced Oct 29, 2024
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.

2 participants