Skip to content

Use ThinColumn in ComponentSparseSet #18245

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 28 commits into from
Closed

Use ThinColumn in ComponentSparseSet #18245

wants to merge 28 commits into from

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Mar 10, 2025 •
edited
Loading

Objective

The Column/ThinColumn split is a significant amount of code duplication and makes optimizations to either harder to maintain.

Solution

  • Migrate ComponentSparseSet to use ThinColumn rather than Column. This requires some extra bookkeeping but it's overall not too bad.

  • Only downside: ComponentSparseSet is no longer Debug.

  • Should we rename ThinColumn to Column now that there's no overlap?

Testing

  • bevy_ecs tests pass
  • miri passes
  • profile to check perf

@ecoskey ecoskey changed the title migrate ComponentSparseSet to ThinColumn Use ThinColumn in ComponentSparseSet Mar 10, 2025
@ecoskey ecoskey added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-Testing Testing must be done before this is safe to merge and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 10, 2025
@ecoskey
Copy link
Contributor Author

ecoskey commented Mar 10, 2025 •
edited
Loading

tests pass!

lastly: Seems like there's some dead code left behind from removing Column, but it seems like the kind of thing we should keep. What's preferred here? removed anyway, can always revert that commit later

@ecoskey ecoskey marked this pull request as ready for review March 10, 2025 23:55
@ecoskey ecoskey added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Needs-Testing Testing must be done before this is safe to merge labels Mar 10, 2025
SpecificProtagonist
SpecificProtagonist reviewed Mar 12, 2025
View reviewed changes
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This isn't a full review yet, but I'm generally in favor this change :)

SpecificProtagonist
SpecificProtagonist approved these changes Mar 12, 2025
View reviewed changes
Copy link
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.

Nice.

@ecoskey ecoskey force-pushed the thin_column_sparse_set branch from 394f0cc to ba457db Compare March 12, 2025 23:03
chescock
chescock approved these changes Mar 14, 2025
View reviewed changes
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

Looks good! Just left a few nits.

  • Should we rename ThinColumn to Column now that there's no overlap?

Renaming it in a follow-up PR sounds good to me!

@ecoskey ecoskey 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 Mar 14, 2025
@ecoskey ecoskey added this to the 0.16 milestone Mar 14, 2025
@alice-i-cecile alice-i-cecile added S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Mar 17, 2025
@alice-i-cecile
Copy link
Member

I have no objections to this in theory, but this 100% needs benchmarking to check for unexpected performance regressions.

@alice-i-cecile alice-i-cecile removed this from the 0.16 milestone Mar 17, 2025
@ItsDoot ItsDoot mentioned this pull request May 9, 2025
Draft
@ecoskey ecoskey force-pushed the thin_column_sparse_set branch from 2a1b5df to 6fe6a7f Compare May 19, 2025 22:24
@james7132
Copy link
Member

Oh shoot, I didn't realize this PR existed before making #20527. @ecoskey, are you still interested in getting this merged? IMO we should go down this route even if there is no performance benefits purely just to reduce the amount of unsafe code we're maintaining.

@james7132 james7132 mentioned this pull request Aug 18, 2025
Open
@ecoskey
Copy link
Contributor Author

ecoskey commented Aug 18, 2025 •
edited
Loading

Still down to merge @james7132 ! I don't mind if you'd rather use your own impl though. I'll fix conflicts this afternoon

@ecoskey
Copy link
Contributor Author

ecoskey commented Aug 18, 2025

I wasn't able to get consistent benchmarks last time I checked but that might have just been my setup being weird

@james7132
Copy link
Member

I don't mind if you'd rather use your own impl though

I think we should go with #20527 then, since it lets us completely remove BlobVec and Column. I'll add you as a co-author to that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@SpecificProtagonist SpecificProtagonist SpecificProtagonist approved these changes

@chescock chescock chescock approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Unsafe Touches with unsafe code in some way S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ecoskey @alice-i-cecile @james7132 @chescock @SpecificProtagonist