Skip to content

use sort_unstable if you're deduplicating anyways#24545

Open
miguelraz wants to merge 1 commit into
bevyengine:mainfrom
miguelraz:sort-unstable-then-dedup
Open

use sort_unstable if you're deduplicating anyways#24545
miguelraz wants to merge 1 commit into
bevyengine:mainfrom
miguelraz:sort-unstable-then-dedup

Conversation

@miguelraz
Copy link
Copy Markdown

@miguelraz miguelraz commented Jun 6, 2026

Objective

  • To make bevy_math faster 🚀

Solution

  • Unstable sorts are potentially destructive, but if you're deduplicating anyways, there's no risk of that, so you might as well use a faster version.

It also helps that it makes less allocations than the normal .sort().

The changes to bevy_ecs's unit tests and examples/ui/text aren't on any critical path, so most of the gains (if any) are for bevy_math.

Testing

  • Did you test these changes? If so, how?

I did cargo test -p bevy_math and got all tests passing:

test result: ok. 117 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.06s

Likewise for cargo test -p bevy_ecs:

test result: ok. 478 passed; 0 failed; 3 ignored; 0 measured; 0 filtered out; finished in 163.98s

  • Are there any parts that need more testing?

No.

  • How can other people (reviewers) test your changes? Is there anything specific they need to know?

Nope.

  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Windows 11, not important.


Showcase

I'd be happy to add a local criterion benchmark if someone points me towards a small representative example to do the before/after comparison - this is my first Bevy PR so I don't know what would be a decent stress test for this code path.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 6, 2026

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

@ChristopherBiscardi ChristopherBiscardi added A-Math Fundamental domain-agnostic mathematical operations S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 6, 2026
Copy link
Copy Markdown

@mardzie mardzie left a comment

Choose a reason for hiding this comment

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

Code looks good to me and the reasoning holds up. I am not familiar with the deeper context though.

Copy link
Copy Markdown
Contributor

@SpecificProtagonist SpecificProtagonist left a comment

Choose a reason for hiding this comment

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

As none of these floats can be NaN, this is correct.

@SpecificProtagonist SpecificProtagonist 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 Jun 6, 2026
@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times C-Code-Quality A section of code that is hard to understand or change labels Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Math Fundamental domain-agnostic mathematical operations C-Code-Quality A section of code that is hard to understand or change 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.

5 participants