Skip to content

Conversation

@JonasJebing
Copy link
Contributor

Objective

  • Fixes ExactSizeIterator::len panic in a few of these iterators. Note: the std default impl of ExactSizeIterator::len has assert_eq!(Some(lower), upper);. I am facing this issue myself with <index_set::Iter as ExactSizeIterator>::len.
  • Should slightly improve performance in some cases, where the more precise size_hint causes less allocations (for example when collecting into a Vec)

Solution

  • Add size_hint overrides that forward to inner iterators or an ExactSizeIterator::len override in one specific case.

Testing

  • Since the overrides are pretty simple, I mostly didn't add any tests.
  • I didn't feel 100% confident in the WriteBatchIds::size_hint implementation, so I added a test there.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@JonasJebing JonasJebing marked this pull request as ready for review December 5, 2025 09:59
Copy link
Contributor

@Victoronz Victoronz left a comment

Choose a reason for hiding this comment

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

Great catch! I was trying to figure out where the performance mismatch I was seeing between these wrappers and their inner types came from, and missing overrides like this, at least in part, look to be a likely answer! (TrustedLen and inlining behavior are some of my other suspects)

Seems like other entity module iterators like UniqueEntityIter and UniqueEntitySliceIter didn't miss out on this specific override, but a general pass should be made through the entire module to properly pipe through trait method overrides.

@Victoronz Victoronz added C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 5, 2025
@JonasJebing
Copy link
Contributor Author

JonasJebing commented Dec 5, 2025

@Victoronz I did do a search for Iterator for in the entire bevy code base and fixed all instances that I saw that way. I'm reasonably confident, that I found all instances out of those search results where the Iterator::next implementation is not more complex than self.0.next().

Edit for clarity: I didn't check whether any other overrides than size_hint would be needed anywhere.

@kristoff3r kristoff3r added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 7, 2025
@mockersf mockersf added this pull request to the merge queue Dec 7, 2025
Merged via the queue into bevyengine:main with commit 89e4193 Dec 7, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

5 participants