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

Reserve space for LocalVector if size is known #100251

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

Conversation

YYF233333
Copy link
Contributor

As stated in 99936(comment), use LocalVector::reserve() if we can.

This covers the core directory. All these cases are trival and should be riskless.

Some length are wrapped with MAX(length, 0) to avoid negative value. Please tell me if these check can be removed.

@YYF233333 YYF233333 requested a review from a team as a code owner December 10, 2024 17:46
@AThousandShips AThousandShips changed the title [Codestyle] Reserve space for LocalVector if size is known Reserve space for LocalVector if size is known Dec 10, 2024
@AThousandShips
Copy link
Member

This is not a codestyle change, just to be clear, this is performance improvements theoretically

@YYF233333
Copy link
Contributor Author

This is not a codestyle change, just to be clear, this is performance improvements theoretically

IMO this should be considered a codestyle improvement before being regarded as a performance optimization. It's a best practice stuff. Not every reserve result in considerable performance gain, but by enforcing this style, we can avoid accidentally writing poorly performing code.

Should probably mention this in code standard, remind everyone to do this.

@AThousandShips
Copy link
Member

We call things that are mostly or only style changes codestyle, otherwise it's very misleading as "this is only surface level"

@@ -340,6 +340,7 @@ Error QuickHull::build(const Vector<Vector3> &p_points, Geometry3D::MeshData &r_
Geometry3D::MeshData::Face f;
f.plane = E.plane;

f.indices.reserve(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole f variable should be removed, as it is only used for the pushback, which will make a copy anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be addressed in another PR, will try impl this ASAP.

@Ivorforce
Copy link
Contributor

IMO this should be considered a codestyle improvement before being regarded as a performance optimization.

If this was a codestyle change instead of an optimization, it would be a bad one because it increases the complexity of the code :)

It's a best practice stuff. Not every reserve result in considerable performance gain, but by enforcing this style, we can avoid accidentally writing poorly performing code.

I agree it's good practice to call reserve when the expected size is known.
And it's good practice because of performance considerations. If it was always of negligible benefit to call reserve, we wouldn't do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants