[Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter #5214

nicopap · 2022-07-05T14:49:53Z

Objective

Solution

Instead of returning the total count of elements in the QueryIter in
size_hint, we return the count of remaining elements. This
Fixes #5149 even when #5148 gets merged.

Changelog

Fix partially consumed QueryIter and QueryCombinationIter having invalid size_hint

nicopap · 2022-07-05T16:42:49Z

We already keep track of which items we iterated over in the QueryIterationCursor, so we might as well use it to compute how many elements there are to iterate over instead of using the full archetype list. It basically cost the same as the current method.

For QueryCombinationIter, we do K binomial coefficients rather than a single one. But I think it's fine, since for each step, we substract 1 from K and N. Also the base case of non-consumed iterator also stays the same as before.

nicopap · 2022-07-05T16:45:19Z

crates/bevy_ecs/src/query/iter.rs

+        fn choose(n: usize, k: usize) -> Option<usize> {
+            if k > n || n == 0 {
+                return Some(0);
+            }
+            let k = k.min(n - k);
+            let ks = 1..=k;
+            let ns = (n + 1 - k..=n).rev();
+            ks.zip(ns)
+                .try_fold(1_usize, |acc, (k, n)| Some(acc.checked_mul(n)? / k))
        }


Same comment as #5148 (comment)

The checked_mul might be overdoing it though, it makes the code harder to read for little benefit

crates/bevy_ecs/src/query/mod.rs

alice-i-cecile · 2022-07-13T14:46:23Z

@nicopap can you update this to reflect the changes that are about to be merged in #5148? Then we can clean up the behavior everywhere at once.

nicopap · 2022-07-13T20:38:29Z

@alice-i-cecile Done now. Should be good to go.

crates/bevy_ecs/src/query/mod.rs

crates/bevy_ecs/src/query/iter.rs

alice-i-cecile

Logic and tests make sense to me. My understanding is that this is off the iteration hot-path, so we can get away with doing the right thing here.

Can you run a before/after on the iteration benchmarks to double-check that nothing funny is happening?

@james7132 I want your opinion on this.

crates/bevy_ecs/src/query/iter.rs

nicopap · 2022-07-14T06:36:31Z

Note that ExactSizeIterator is not that much of a performance win. collect used the minimum bound of size_hint already for pre-allocating. It's just (but still pretty good) an ergonomic win. This still doesn't enable optimizing away checks for reallocation (even if we know it will never happen) because it's safe API. TrustedLen could really be helpful here, and the way overflow is handled in the combination code is compliant with the TrustedLen API. However, TrustedLen is still unstable and we cannot use it.

Tracking issue for TrustedLen (trusted_len) rust-lang/rust#37572

nicopap · 2022-07-14T13:00:56Z

Here are the criterion results https://nicopap.ch/p/bevy_exact_size_iter_criterion_results/all_changes.html

The benchmark ran on a very clean machine with a minimal amount of concurrent processes. I've noticed they were fairly consistent with regard to reproducibility. But multithreading seems to be very noisy on my machine, no idea why.

alice-i-cecile · 2022-07-14T15:42:17Z

Thanks for the criterion results. I'm happy to call that a wash; multithreading is noisy and there's no critical regressions.

hymm · 2022-07-21T21:32:55Z

Here are the criterion results https://nicopap.ch/p/bevy_exact_size_iter_criterion_results/all_changes.html

I don't think any of these benches shown above use the query iterator. Do you have results for the benches in this folder? https://github.com/bevyengine/bevy/tree/main/benches/benches/bevy_ecs/iteration

edit: I also don't think we have any benches for iter combinations, which I would be more worried about the performance of.

crates/bevy_ecs/src/query/iter.rs

nicopap · 2022-07-22T06:40:38Z

I don't think any of these benches shown above use the query iterator. Do you have results for the benches in this folder? https://github.com/bevyengine/bevy/tree/main/benches/benches/bevy_ecs/iteration

Wow my bad! I updated the page with the actual ECS iteration benches. The bench are a bit more consistent.

It seems I did the benches for the bevy_tasks/iter.rs previously. It's very surprising that a change that supposedly has nothing related would result in bench differences of up to 30% (in both direction). Wouldn't that make the benches unreliable? Would it not be better to delete them altogether if they cannot be trusted?

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149. This is also true of `QueryCombinationIter`. - bevyengine#5149 - bevyengine#5148

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

nicopap · 2022-11-01T11:32:12Z

crates/bevy_ecs/src/query/mod.rs

@@ -57,100 +57,6 @@ mod tests {

    #[test]
    fn query_filtered_exactsizeiterator_len() {


Note that this test was redundant with query_filtered_combination_size because we merged the two, and is why it's deleted.

james7132

Took a bit to review the math, but LGTM.

crates/bevy_ecs/src/query/iter.rs

alice-i-cecile

bors r+

…er (#5214) # Objective Fix #5149 ## Solution Instead of returning the **total count** of elements in the `QueryIter` in `size_hint`, we return the **count of remaining elements**. This Fixes #5149 even when #5148 gets merged. - #5149 - #5148 --- ## Changelog - Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint` Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

bors · 2022-11-21T12:58:57Z

Pull request successfully merged into main.

Build succeeded:

…er (#5214) # Objective Fix #5149 ## Solution Instead of returning the **total count** of elements in the `QueryIter` in `size_hint`, we return the **count of remaining elements**. This Fixes #5149 even when #5148 gets merged. - #5149 - #5148 --- ## Changelog - Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint` Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

…er (bevyengine#5214) # Objective Fix bevyengine#5149 ## Solution Instead of returning the **total count** of elements in the `QueryIter` in `size_hint`, we return the **count of remaining elements**. This Fixes bevyengine#5149 even when bevyengine#5148 gets merged. - bevyengine#5149 - bevyengine#5148 --- ## Changelog - Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint` Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jul 5, 2022

alice-i-cecile mentioned this pull request Jul 5, 2022

[Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter #5148

Closed

nicopap force-pushed the fix-exactsize branch 2 times, most recently from ee4a84b to c94a1b3 Compare July 5, 2022 16:16

nicopap changed the title ~~Fix size_hint for partially consumed QueryIter~~ Fix size_hint for partially consumed QueryIter and QueryCombinationIter Jul 5, 2022

nicopap force-pushed the fix-exactsize branch from c94a1b3 to 14173c2 Compare July 5, 2022 16:28

nicopap commented Jul 5, 2022

View reviewed changes

crates/bevy_ecs/src/query/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile requested review from alice-i-cecile and james7132 July 13, 2022 14:45

nicopap force-pushed the fix-exactsize branch from 14173c2 to 26adeab Compare July 13, 2022 20:27

nicopap commented Jul 13, 2022

View reviewed changes

crates/bevy_ecs/src/query/mod.rs Show resolved Hide resolved

alice-i-cecile reviewed Jul 14, 2022

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 14, 2022

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 14, 2022

View reviewed changes

nicopap commented Jul 14, 2022

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved

nicopap force-pushed the fix-exactsize branch from 34234c8 to 7f2c3b2 Compare July 19, 2022 13:52

alice-i-cecile added this to the Bevy 0.8 milestone Jul 19, 2022

BoxyUwU reviewed Jul 21, 2022

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Show resolved Hide resolved

nicopap force-pushed the fix-exactsize branch from 7f2c3b2 to 46400ba Compare July 22, 2022 07:13

cart removed this from the Bevy 0.8 milestone Jul 30, 2022

nicopap force-pushed the fix-exactsize branch from 46400ba to 3a4c633 Compare September 1, 2022 09:08

nicopap and others added 3 commits November 1, 2022 11:55

Fix comment note

1c0b66d

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Rename remaning to be more explicit

5ef7e5c

nicopap force-pushed the fix-exactsize branch from 3a4c633 to 5ef7e5c Compare November 1, 2022 11:16

alice-i-cecile requested a review from BoxyUwU November 1, 2022 11:29

alice-i-cecile approved these changes Nov 1, 2022

View reviewed changes

nicopap commented Nov 1, 2022

View reviewed changes

james7132 mentioned this pull request Nov 12, 2022

QueryIter implements ExactSizeIterator, but doesnt provide an exact size_hint #1686

Closed

james7132 approved these changes Nov 15, 2022

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 15, 2022

james7132 added this to the 0.9.1 milestone Nov 15, 2022

nicopap commented Nov 15, 2022

View reviewed changes

crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved

Swap back binomial formula arguments

ce115ff

alice-i-cecile approved these changes Nov 21, 2022

View reviewed changes

bors bot changed the title ~~Fix size_hint for partially consumed QueryIter and QueryCombinationIter~~ [Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter Nov 21, 2022

bors bot closed this Nov 21, 2022

nicopap deleted the fix-exactsize branch September 5, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter #5214

[Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter #5214

nicopap commented Jul 5, 2022 •

edited

Loading

nicopap commented Jul 5, 2022

nicopap Jul 5, 2022

alice-i-cecile commented Jul 13, 2022

nicopap commented Jul 13, 2022

alice-i-cecile left a comment

nicopap commented Jul 14, 2022

nicopap commented Jul 14, 2022 •

edited

Loading

alice-i-cecile commented Jul 14, 2022

hymm commented Jul 21, 2022 •

edited

Loading

nicopap commented Jul 22, 2022

nicopap Nov 1, 2022

james7132 left a comment

alice-i-cecile left a comment

bors bot commented Nov 21, 2022

		@@ -57,100 +57,6 @@ mod tests {

		#[test]
		fn query_filtered_exactsizeiterator_len() {

[Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter #5214

[Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter #5214

Conversation

nicopap commented Jul 5, 2022 • edited Loading

Objective

Solution

Changelog

nicopap commented Jul 5, 2022

nicopap Jul 5, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Jul 13, 2022

nicopap commented Jul 13, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

nicopap commented Jul 14, 2022

nicopap commented Jul 14, 2022 • edited Loading

alice-i-cecile commented Jul 14, 2022

hymm commented Jul 21, 2022 • edited Loading

nicopap commented Jul 22, 2022

nicopap Nov 1, 2022

Choose a reason for hiding this comment

james7132 left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

bors bot commented Nov 21, 2022

nicopap commented Jul 5, 2022 •

edited

Loading

nicopap commented Jul 14, 2022 •

edited

Loading

hymm commented Jul 21, 2022 •

edited

Loading