Skip to content

[Merged by Bors] - remove Box from ExclusiveSystemFn#3063

Closed
R3DP1X37 wants to merge 2 commits intobevyengine:mainfrom
R3DP1X37:main
Closed

[Merged by Bors] - remove Box from ExclusiveSystemFn#3063
R3DP1X37 wants to merge 2 commits intobevyengine:mainfrom
R3DP1X37:main

Conversation

@R3DP1X37
Copy link
Contributor

@R3DP1X37 R3DP1X37 commented Nov 3, 2021

Minor refactor to remove the boxing of the function pointer stored in ExclusiveSystemFn.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Nov 3, 2021
@alice-i-cecile 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 S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Nov 3, 2021
@R3DP1X37 R3DP1X37 mentioned this pull request Nov 3, 2021
Merged
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 3, 2021

Interesting, thanks. So this increases monomorphization, but exclusive systems are relatively rare.

This should very slightly hurt compile times, but somewhat improve runtime performance? That seems like a fine tradeoff.

My main concern is that doing this means that runtime system insertion might become harder down the line. However, it looks like we already do the same thing for non-exclusive systems, so this has my approval.

@R3DP1X37
Copy link
Contributor Author

R3DP1X37 commented Nov 3, 2021

Thanks for the quick review Alice!

DJMcNab
DJMcNab approved these changes Nov 3, 2021
View reviewed changes
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

This looks correct, and I agree that this change makes sense. The double indirection would otherwise be very unfortunate.

(I don't like that I don't trust our testing infrastructure to know whether exclusive systems not working would be caught, but that's not this PR's problem)

@alice-i-cecile alice-i-cecile 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 labels Nov 3, 2021
@cart
Copy link
Member

cart commented Nov 4, 2021

bors r+

bors bot pushed a commit that referenced this pull request Nov 4, 2021
@R3DP1X37
remove Box from ExclusiveSystemFn (#3063)
225d6a1 
Minor refactor to remove the boxing of the function pointer stored in ExclusiveSystemFn.
@bors
Copy link
Contributor

bors bot commented Nov 4, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title remove Box from ExclusiveSystemFn [Merged by Bors] - remove Box from ExclusiveSystemFn Nov 4, 2021
@bors bors bot closed this Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+2 more reviewers

@bjorn3 bjorn3 bjorn3 approved these changes

@DJMcNab DJMcNab DJMcNab approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@R3DP1X37 @alice-i-cecile @cart @bjorn3 @DJMcNab