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

Size hint for Chunks is wrong #2610

Open
Darksonn opened this issue Jun 5, 2022 · 2 comments · Fixed by #2611
Open

Size hint for Chunks is wrong #2610

Darksonn opened this issue Jun 5, 2022 · 2 comments · Fixed by #2611
Labels
A-stream Area: futures::stream bug

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Jun 5, 2022

The size hint for chunks is computed as follows:

fn size_hint(&self) -> (usize, Option<usize>) {
    let chunk_len = if self.items.is_empty() { 0 } else { 1 };
    let (lower, upper) = self.stream.size_hint();
    let lower = lower.saturating_add(chunk_len);
    let upper = match upper {
        Some(x) => x.checked_add(chunk_len),
        None => None,
    };
    (lower, upper)
}

However, this is incorrect. The stream combines many items into one, so it should divide the size hints by the capacity.

@SaltyKitkat
Copy link

In #2611, lower in size_hint is fixed. But upper is untouched. I wonder if we should also change x.checked_add(chunk_len) to (x / self.cap).checked_add(chunk_len) or something like that.

@taiki-e taiki-e reopened this Mar 23, 2024
@SaltyKitkat
Copy link

SaltyKitkat commented Mar 23, 2024

Maybe size_hint for chunks should be something like this:

    fn size_hint(&self) -> (usize, Option<usize>) {
        let (lower, upper) = self.stream.size_hint();
        let len = self.items.len();
        let lower = lower.saturating_add(len) / self.cap;
        let upper = upper.and_then(|u| u.checked_add(len)).map(|u| u / self.cap);
        (lower, upper)
    }

Edit: and we should use div_ceil instead of /

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stream Area: futures::stream bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants