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

Reduce compile time of bevy_ptr::OwnedPtr::make function #15644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Oct 4, 2024

Methodology

A good metric that correlates with compile time is the amount of code generated by the compiler itself; even if the end binary is exactly the same size, having more copies of the same code can really slow down compile time, since it has to figure out whether it needs to include them or not.

The measurement for this used was the cargo-llvm-lines crate which can measure which functions are generating the most lines of LLVM IR, which generally means more code compiled. The example compiled was the breakout game, to choose something that touches a decent portion of the engine.

Solution

Based upon the measurements, bevy_ptr::OwnedPtr::make was taking up 4061 lines of LLVM IR in the example code. So, I separated part of this function into a less-monomorphised version to reduce the amount of generated code. This was by far the most lines emitted by any single function.

Results

After this change, only 2560 lines are emitted, accounting for a 36% decrease. I tried timing the results and it seemed like it did decrease compile times a bit, but honestly, the data is really noisy and I can't be bothered to compile bevy for hours on end to get enough data points.

The tweak feels like an improvement, so, I'll offer it, however small.

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

It can be quite hard to measure these smaller changes, but I agree with the technique, and it's trivial for the compiler to inline the new function if that produces better results.

@bushrat011899 bushrat011899 added D-Trivial Nice and easy! A great choice to get started with Bevy C-Code-Quality A section of code that is hard to understand or change A-Pointers Relating to Bevy pointer abstractions X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 4, 2024
Comment on lines 411 to 415
// SAFETY: The value behind the pointer will not get dropped or observed later,
// so it's safe to promote it to an owning pointer.
Copy link
Member

Choose a reason for hiding this comment

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

This can probably be removed now, right? Since the unsafe code was moved into make_internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I tried multiple ways to rearrange the code and forgot to remove the comment on the latest one.

Copy link
Contributor

Choose a reason for hiding this comment

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

make_internal should be an unsafe method. The safety is still unheld in this method.

@@ -397,13 +397,20 @@ impl<'a, T: ?Sized> From<&'a mut T> for PtrMut<'a> {
}

impl<'a> OwningPtr<'a> {
/// This exists mostly to reduce compile times;
/// code is only duplicated per type, rather than per function called.
fn make_internal<T>(temp: &mut ManuallyDrop<T>) -> OwningPtr<'_> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an unsafe method and have the same safety requirements as the promote method.

/// This exists mostly to reduce compile times;
/// code is only duplicated per type, rather than per function called.
fn make_internal<T>(temp: &mut ManuallyDrop<T>) -> OwningPtr<'_> {
// SAFETY: The value behind the pointer will not get dropped or observed later,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// SAFETY: The value behind the pointer will not get dropped or observed later,
// SAFETY: Safety is unheld by the caller.

The following line should be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

I get making the function itself unsafe, but what value does this safety comment add? If safety is unheld, shouldn't we not be using unsafe code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops typo. It should be upheld

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay that makes sense haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The caller lets go of all safety constraints and becomes free.

Comment on lines 411 to 415
// SAFETY: The value behind the pointer will not get dropped or observed later,
// so it's safe to promote it to an owning pointer.
Copy link
Contributor

Choose a reason for hiding this comment

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

make_internal should be an unsafe method. The safety is still unheld in this method.

@clarfonthey
Copy link
Contributor Author

I fixed make_internal not being marked as unsafe (oops) and hopefully things should be okay now. Please let me know if this looks correct, since I don't really understand these types.

@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Pointers Relating to Bevy pointer abstractions 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 X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants