[Merged by Bors] - Improve QueryIter size_hint hints #4244

nicopap · 2022-03-17T20:09:02Z

Objective

This fixes #1686.

size_hint can be useful even if a little niche. For example,
collect::<Vec<_>>() uses the size_hint of Iterator it collects from
to pre-allocate a memory slice large enough to not require re-allocating
when pushing all the elements of the iterator.

Solution

To this effect I made the following changes:

Add a IS_ARCHETYPAL associated constant to the Fetch trait,
this constant tells us when it is safe to assume that the Fetch
relies exclusively on archetypes to filter queried entities
Add IS_ARCHETYPAL to all the implementations of Fetch
Use that constant in QueryIter::size_hint to provide a more useful

Migration guide

The new associated constant is an API breaking change. For the user,
if they implemented a custom Fetch, it means they have to add this
associated constant to their implementation. Either true if it doesn't limit
the number of entities returned in a query beyond that of archetypes, or
false for when it does.

MinerSebas · 2022-03-17T20:40:46Z

The size_hint of the QueryCombinationIter should also be updated (https://github.com/nicopap/bevy/blob/filter-size-hint/crates/bevy_ecs/src/query/iter.rs#L331)

The ExactSizeIterator Implementation (https://github.com/nicopap/bevy/blob/filter-size-hint/crates/bevy_ecs/src/query/iter.rs#L335-L351) also needs to be updated.

nicopap · 2022-03-17T20:55:46Z

The size_hint of the QueryCombinationIter should also be updated (https://github.com/nicopap/bevy/blob/filter-size-hint/crates/bevy_ecs/src/query/iter.rs#L331)

Technically it doesn't need to (I don't see an ExactSizeIterator for QueryCombinationIter), but I can add that functionality. I might look into that tomorrow if I have time.

The ExactSizeIterator Implementation (https://github.com/nicopap/bevy/blob/filter-size-hint/crates/bevy_ecs/src/query/iter.rs#L335-L351) also needs to be updated.

It doesn't make sense to check in ExactSizeIterator. Since the trait iterator has an exact length. You can't do a min_size = if {... since ExactSizeIterator::len is a unique value and not a tuple like Iterator::size_hint. The code doesn't change, it's still the same.

crates/bevy_ecs/src/query/iter.rs

alice-i-cecile

Code LGTM. I'd be happy to get the combinations work done at the same time, but I won't block on it. The IS_ARCHETYPAL associated const is solid: I suspect we'll run into more uses for this later.

nicopap · 2022-03-18T06:04:19Z

Perfect. I think I can add fairly trivially the combination code, and I'll also remove the outdated references to #1686 in the comments.

nicopap · 2022-03-18T08:21:50Z

uhh, the tests assumes minimal size hint is 0, I'll update them.

mockersf · 2022-03-18T08:30:54Z

looks good once the tests are fixed 👍

nicopap · 2022-03-19T08:52:31Z

Should be good to go now. I updated the tests so that they don't inspect the min size of the QueryCombinatorIter. Everything passes.

alice-i-cecile · 2022-04-26T19:15:31Z

bors r+

## Objective This fixes #1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. ## Solution To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful ## Migration guide The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation. Either `true` if it doesn't limit the number of entities returned in a query beyond that of archetypes, or `false` for when it does.

bors · 2022-04-26T19:41:18Z

Build failed (retrying...):

build (stable, macos-latest)

## Objective This fixes #1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. ## Solution To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful ## Migration guide The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation. Either `true` if it doesn't limit the number of entities returned in a query beyond that of archetypes, or `false` for when it does.

bors · 2022-04-26T20:47:08Z

Build failed (retrying...):

build (stable, macos-latest)

## Objective This fixes #1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. ## Solution To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful ## Migration guide The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation. Either `true` if it doesn't limit the number of entities returned in a query beyond that of archetypes, or `false` for when it does.

bors · 2022-04-26T21:20:05Z

Build failed (retrying...):

build (stable, macos-latest)

## Objective This fixes #1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. ## Solution To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful ## Migration guide The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation. Either `true` if it doesn't limit the number of entities returned in a query beyond that of archetypes, or `false` for when it does.

bors · 2022-04-26T21:51:42Z

Build failed:

build (stable, macos-latest)

alice-i-cecile · 2022-04-26T22:12:29Z

bors try

bors · 2022-04-26T22:22:42Z

try

Build failed:

build (stable, windows-latest)

alice-i-cecile · 2022-04-26T22:54:10Z

@nicopap looks like you've got some failing tests now. Could you rebase and fix that up?

nicopap · 2022-04-27T07:41:37Z

Working on it. Aaah, having some computer related woes right now. I'll leave a thank to BORS for doing its job yet again!

This fixes bevyengine#1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful minima when it's possible to predict the exact size of the query. The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation.

See commit 7d92ba5 for rationale.

nicopap · 2022-04-27T09:00:08Z

I took the opportunity to rename some variables in the test to make the code clearer (the names are not clearer, but with rustfmt, the structure of the code is much easier to grasp at a quick glance).

@alice-i-cecile should be good to go 👍

alice-i-cecile · 2022-04-27T17:49:41Z

bors r+

## Objective This fixes #1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. ## Solution To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful ## Migration guide The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation. Either `true` if it doesn't limit the number of entities returned in a query beyond that of archetypes, or `false` for when it does.

bors · 2022-04-27T18:31:58Z

Pull request successfully merged into main.

Build succeeded:

## Objective This fixes bevyengine#1686. `size_hint` can be useful even if a little niche. For example, `collect::<Vec<_>>()` uses the `size_hint` of Iterator it collects from to pre-allocate a memory slice large enough to not require re-allocating when pushing all the elements of the iterator. ## Solution To this effect I made the following changes: * Add a `IS_ARCHETYPAL` associated constant to the `Fetch` trait, this constant tells us when it is safe to assume that the `Fetch` relies exclusively on archetypes to filter queried entities * Add `IS_ARCHETYPAL` to all the implementations of `Fetch` * Use that constant in `QueryIter::size_hint` to provide a more useful ## Migration guide The new associated constant is an API breaking change. For the user, if they implemented a custom `Fetch`, it means they have to add this associated constant to their implementation. Either `true` if it doesn't limit the number of entities returned in a query beyond that of archetypes, or `false` for when it does.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 17, 2022

nicopap mentioned this pull request Mar 17, 2022

[Merged by Bors] - Rework extract_meshes #4240

Closed

nicopap force-pushed the filter-size-hint branch from 36d2534 to 7d92ba5 Compare March 17, 2022 20:27

alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 17, 2022

alice-i-cecile reviewed Mar 17, 2022

View reviewed changes

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

alice-i-cecile approved these changes Mar 17, 2022

View reviewed changes

mockersf approved these changes Mar 18, 2022

View reviewed changes

nicopap force-pushed the filter-size-hint branch from 1c31ee4 to 4650705 Compare March 18, 2022 08:30

alice-i-cecile mentioned this pull request Mar 23, 2022

[Merged by Bors] - Filter material handles on extraction #4178

Closed

alice-i-cecile 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 Mar 23, 2022

alice-i-cecile mentioned this pull request Mar 24, 2022

High entity counts cause decreased rendering performance even when rendering nothing. #3953

Closed

nicopap mentioned this pull request Apr 15, 2022

bevy_render: Reserve only entities from the last frame #4322

Closed

bors bot added a commit that referenced this pull request Apr 26, 2022

Try #4244:

61663e4

nicopap added 3 commits April 27, 2022 09:44

Add better size_hint to QueryCombinationIter

d4dd12d

See commit 7d92ba5 for rationale.

Update query/iter comments related to size_hint

6d6a106

nicopap force-pushed the filter-size-hint branch from 4650705 to 6d6a106 Compare April 27, 2022 08:38

bors bot changed the title ~~Improve QueryIter size_hint hints~~ [Merged by Bors] - Improve QueryIter size_hint hints Apr 27, 2022

bors bot closed this Apr 27, 2022

alice-i-cecile mentioned this pull request Jun 4, 2022

flappy bevy example #4923

Closed

james7132 mentioned this pull request Nov 12, 2022

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

Closed

nicopap deleted the filter-size-hint branch September 5, 2023 15:43

Uh oh!

[Merged by Bors] - Improve QueryIter size_hint hints #4244

[Merged by Bors] - Improve QueryIter size_hint hints #4244

Uh oh!

Conversation

nicopap commented Mar 17, 2022

Objective

Solution

Migration guide

Uh oh!

MinerSebas commented Mar 17, 2022

Uh oh!

nicopap commented Mar 17, 2022

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

nicopap commented Mar 18, 2022

Uh oh!

nicopap commented Mar 18, 2022

Uh oh!

mockersf commented Mar 18, 2022

Uh oh!

nicopap commented Mar 19, 2022

Uh oh!

alice-i-cecile commented Apr 26, 2022

Uh oh!

bors bot commented Apr 26, 2022

Uh oh!

bors bot commented Apr 26, 2022

Uh oh!

bors bot commented Apr 26, 2022

Uh oh!

bors bot commented Apr 26, 2022

Uh oh!

alice-i-cecile commented Apr 26, 2022

Uh oh!

bors bot commented Apr 26, 2022

try

Uh oh!

alice-i-cecile commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicopap commented Apr 27, 2022

Uh oh!

nicopap commented Apr 27, 2022

Uh oh!

alice-i-cecile commented Apr 27, 2022

Uh oh!

bors bot commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alice-i-cecile commented Apr 26, 2022 •

edited

Loading