Skip to content

Convert while-pop_front loops to iterator loops #104928

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

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

Conversation

YYF233333
Copy link
Contributor

Cleanup loops which are linear scan but use while.

Semantically, this change provides greater optimization potential because we can now batch free elements. However, this benefit can only be realized after switching the container to Vector/LocalVector, as List always removes elements one by one.

Some loops are at the bottom of their scope and the container destruct right after the loop, I omit clear() for them.

@YYF233333 YYF233333 requested review from a team as code owners April 2, 2025 15:33
@AThousandShips AThousandShips changed the title Convert while-pop_front loop to iterator loop Convert while-pop_front loops to iterator loops Apr 2, 2025
@AThousandShips AThousandShips added this to the 4.x milestone Apr 2, 2025
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

godot/core/templates/list.h

Lines 467 to 471 in 1f56d96

void clear() {
while (front()) {
erase(front());
}
}

For List this will just add another unnecessary loop, which might be optimized out by compiler, but definitely need performance testing.

@Ivorforce
Copy link
Member

godot/core/templates/list.h

Lines 467 to 471 in 1f56d96

void clear() {
while (front()) {
erase(front());
}
}

For List this will just add another unnecessary loop, which might be optimized out by compiler, but definitely need performance testing.

I don't think the compiler can optimize this out, as it would violate code execution order.
With lists, it probably is faster to do a while - pop_front than iteration.
In rust, this would be conveniently solved with a into_iterator(), but I don't think there's a clean way to do this in C++.

@YYF233333
Copy link
Contributor Author

I update the "other half" of this PR which replace these List with LocalVector. Now it is 2 iteration + 1 memfree v.s. 1 iteration + N memfree, and I can say generally this will be faster.

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