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

Add explicit ZeroVec and VarZeroVec iterator types #4487

Open
sffc opened this issue Dec 22, 2023 · 4 comments
Open

Add explicit ZeroVec and VarZeroVec iterator types #4487

sffc opened this issue Dec 22, 2023 · 4 comments
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake question Unresolved questions; type unclear
Milestone

Comments

@sffc
Copy link
Member

sffc commented Dec 22, 2023

The standard library exports concrete types for iterators. However, our ZeroVec types only return impl Iterator. This limits a certain amount of flexibility; for example, you can't put multiple iterators together into a list because the compiler doesn't know that the impl Iterators are going to be the same type.

I also question whether impl Iterator is highly efficient in terms of code size and performance. I remember pulling apart some spaghetti iterators into procedural code and getting a boost. Returning a concrete type could make code simpler to compile.

Thoughts? @Manishearth

@sffc sffc added needs-approval One or more stakeholders need to approve proposal C-zerovec Component: Yoke, ZeroVec, DataBake labels Dec 22, 2023
@sffc sffc added this to the Utilities 1.0 milestone Dec 22, 2023
@Manishearth
Copy link
Member

This was a deliberate choice so as not to overcommit to the implementation of the iterator, especially since we are returning adapters.

We can write explicit adapters, however:

  • we should make them opaque types like ZeroVecSliceIter
  • we need to be wary of perf losses since doing this transform requires somewhat unrolling the iterator chain, which is sometimes harder to optimize without unsafe

you can't put multiple iterators together into a list because the compiler doesn't know that the impl Iterators are going to be the same type

The compiler should allow you to do this as long as both iterators come from the same method. It's intermediate methods that cause this problem.

I also question whether impl Iterator is highly efficient in terms of code size and performance. I remember pulling apart some spaghetti iterators into procedural code and getting a boost. Returning a concrete type could make code simpler to compile.

This is simply not the case; that's not how impl Iterator works. I suspect your boost was due to making things procedural, not because of impl Iterator vs explicit iterators.

@sffc sffc added question Unresolved questions; type unclear and removed needs-approval One or more stakeholders need to approve proposal labels Dec 22, 2023
@sffc
Copy link
Member Author

sffc commented Dec 22, 2023

The compiler should allow you to do this as long as both iterators come from the same method. It's intermediate methods that cause this problem.

Yes and I have intermediate methods. I worked around the problem in my datetime PR by using the as_ule_slice() escape hatch, which I am glad we expose, instead of ZeroVec's own iterators.

I suspect your boost was due to making things procedural, not because of impl Iterator vs explicit iterators.

What I was referring to is #2603. What I saw in the bytecode while working on that change was a mess of iterator abstractions (zip, chain, ...) which went away when I rewrote the code using a for loop.

I wanted to discuss the ZeroVec iterators because they are currently written as

    pub fn iter(&self) -> impl DoubleEndedIterator<Item = T> + ExactSizeIterator<Item = T> + '_ {
        self.as_ule_slice().iter().copied().map(T::from_unaligned)
    }

That impl is probably fine because I think copied and map are most likely zero-cost abstractions. My problems previously were specifically with zip and chain which are not zero-cost.

@sffc
Copy link
Member Author

sffc commented Dec 22, 2023

Also it makes it trickier to use with trait types that need a concrete type. I think a very recent Rust addresses this (rust-lang/rust#91611) but it is not MSRV yet.

EDIT: Actually it is rust-lang/rust#63063 which is not stable yet.

@Manishearth
Copy link
Member

I wanted to discuss the ZeroVec iterators because they are currently written as

Ah, that's a different problem than impl Trait.

I would measure this. 99% of the time iterator chains are more efficient than loop iteration.


Either way, I'm in favor of using concrete named types; I just didn't choose to do this initially. We should be very careful to give them a limited API though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake question Unresolved questions; type unclear
Projects
None yet
Development

No branches or pull requests

2 participants