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

Replace OrdOffset with OffsetList #437

Merged

Conversation

frankmcsherry
Copy link
Member

The OrdOffset trait allowed us to slot in u32 or u64 in for usize for offsets, in order to save the RAMs. That isn't terrible, but we can remove the complexity, and the potential surprise if we go too large, with the OffsetList struct that generally behaves as a Vec<u32> until we reach a point at which the entries exceed its size, at which point it behaves like a not-great Vec<u64>.

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.

Nice!

src/trace/implementations/mod.rs Show resolved Hide resolved
src/trace/implementations/mod.rs Show resolved Hide resolved
@frankmcsherry frankmcsherry merged commit 3182200 into TimelyDataflow:master Nov 28, 2023
1 check passed
@frankmcsherry frankmcsherry deleted the remove_ord_offset branch November 28, 2023 21:00
@frankmcsherry
Copy link
Member Author

Parting thought: if we ever don't like the opinionated lock-in of OffsetList, it should be easy to add back special cases that just use one type and panics when things don't work (to recover the prior implementation).

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