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

Further trait reorganization #427

Merged

Conversation

frankmcsherry
Copy link
Member

This PR moves BatchContainer and OrdOffset out of the layers module which is specific to Trie implementations, and in to the implementations module, which seems more appropriate.

It also experiments a bit with the BatchContainer trait, allowing Item to be unsized, and supporting push and copy_slice only when T: ToOwned. This is .. unfortunately a restriction rather than a pure generalization; to implement ToOwned you have to be able to clone yourself, even though we don't need this.

Up for discussion, but I think based on the vibes around containers, and their non-ownership-providing nature, this might be ok.

cc: @antiguru

@frankmcsherry
Copy link
Member Author

Oh oops, it also adds a SliceContainer which shows off the new BatchContainer by having an Item = [B] but accepting Vec<B> in push and copy_slice, and storing only one Vec<B> and a whole bunch of offsets into it (as many as you introduce, plus one).

@frankmcsherry frankmcsherry merged commit 0dded6b into TimelyDataflow:master Nov 23, 2023
1 check passed
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 fine!

inner: Vec::with_capacity(size),
}
}
fn reserve(&mut self, _additional: usize) {
Copy link
Member

Choose a reason for hiding this comment

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

Note to @antiguru: we could have a slightly better implementation here to pre-size the local part of self, but I can't recall the available apis out of my head.

}

// The `ToOwned` requirement exists to satisfy `self.reserve_items`, who must for now
// be presented with the actual contained type, rather than a type that borrows into it.
Copy link
Member

Choose a reason for hiding this comment

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

We should revisit the type requirements on region and timely stack!

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! I'm not sure if it really helps here, but e.g. if I have a bunch of Vec<u8> and you are able to only maintain [u8] shaped data, that could help. Or we could conclude that this type does not do that, and not be too stressed! :D

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