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

[Merged by Bors] - Optimize .nth() and .last() for event iterators #7530

Closed
wants to merge 9 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Feb 6, 2023

Objective

Motivated by #7469.

EventReader iterators use the default implementations for .nth() and .last(), which includes iterating over and throwing out all events before the desired one.

Solution

Add specialized implementations for these methods that directly updates the unread event counter and returns a reference to the desired event.

TODO:

  • Add a unit test.
  • Add a benchmark, to see if the compiler was doing this automatically already. On second thought, this doesn't feel like a very useful thing to include in the benchmark suite.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 6, 2023
@JoJoJet JoJoJet marked this pull request as ready for review February 7, 2023 13:30
@JoJoJet JoJoJet requested a review from james7132 February 7, 2023 18:58
bors bot pushed a commit that referenced this pull request Feb 13, 2023
# Objective

Related to #7530.

`EventReader` iterators currently use the default impl for `.count()`, which unnecessarily loops over all unread events.

# Solution

Add specialized impls that mark the `EventReader` as consumed and return the number of unread events.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 14, 2023
# Objective

Related to bevyengine#7530.

`EventReader` iterators currently use the default impl for `.count()`, which unnecessarily loops over all unread events.

# Solution

Add specialized impls that mark the `EventReader` as consumed and return the number of unread events.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
# Objective

Related to bevyengine#7530.

`EventReader` iterators currently use the default impl for `.count()`, which unnecessarily loops over all unread events.

# Solution

Add specialized impls that mark the `EventReader` as consumed and return the number of unread events.
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Took longer to look into this. Wanted to make sure the overridden implementation on Chain and slice's iterators were optimized as mentioned in the PR description. LGTM.

@james7132 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 Feb 16, 2023
@james7132
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 16, 2023
# Objective

Motivated by #7469.

`EventReader` iterators use the default implementations for `.nth()` and `.last()`, which includes iterating over and throwing out all events before the desired one.

## Solution

Add specialized implementations for these methods that directly updates the unread event counter and returns a reference to the desired event.

TODO:

- [x] Add a unit test.
- [x] ~~Add a benchmark, to see if the compiler was doing this automatically already.~~ *On second thought, this doesn't feel like a very useful thing to include in the benchmark suite.*
@bors bors bot changed the title Optimize .nth() and .last() for event iterators [Merged by Bors] - Optimize .nth() and .last() for event iterators Feb 16, 2023
@bors bors bot closed this Feb 16, 2023
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 18, 2023
# Objective

Motivated by bevyengine#7469.

`EventReader` iterators use the default implementations for `.nth()` and `.last()`, which includes iterating over and throwing out all events before the desired one.

## Solution

Add specialized implementations for these methods that directly updates the unread event counter and returns a reference to the desired event.

TODO:

- [x] Add a unit test.
- [x] ~~Add a benchmark, to see if the compiler was doing this automatically already.~~ *On second thought, this doesn't feel like a very useful thing to include in the benchmark suite.*
@JoJoJet JoJoJet deleted the optimize-last branch September 20, 2023 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants