Skip to content

Optimize find in Array and CowData #84595

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

Closed
wants to merge 1 commit into from

Conversation

Tom3s
Copy link

@Tom3s Tom3s commented Nov 7, 2023

The array's find implementation looped through all elements, even after finding a corresponding element. On one hand this results in finding the last element (which is already available by rfind), but is also ineffient and can cause issues, when a partial element is followed by a complete element (finding the partial one can cause runtime errors)

To clarify, I was using a comment indenting algorithm, that indented each comment based on their parent comment's level of indentation. It was known, that the parent comment will be positioned in the array, before any element without the indentation field (that's what I referred to as partial data) but this implementation caused an error

The array's find implementation looped through all elements, even after finding a corresponding element. On one hand this results in finding the last element (which is already available by rfind), but is also ineffient and can cause issues, when a partial element is followed by a complete element (finding the partial one can cause runtime errors)

To clarify, I was using a comment indenting algorithm, that indented each comment based on their parent comment's level of indentation. It was known, that the parent comment will be positioned in the array, before any element without the indentation field (that's what I referred to as partial data) but this implementation caused an error
@Tom3s Tom3s requested a review from a team as a code owner November 7, 2023 22:40
@AThousandShips
Copy link
Member

AThousandShips commented Nov 8, 2023

The array's find implementation looped through all elements, even after finding a corresponding element

This is false, you are missing the break which stops the loop

This change can be an improvement, but the current code isn't wrong

However I don't think this will result in any improvement, as any decent optimizing compiler will optimize out this aspect on its own

@AThousandShips AThousandShips added this to the 4.x milestone Nov 8, 2023
@AThousandShips AThousandShips changed the title Fixed incorrect find in array.cpp and cowdata.h Optimize find in Array and CowData Nov 8, 2023
@Tom3s
Copy link
Author

Tom3s commented Nov 8, 2023

The array's find implementation looped through all elements, even after finding a corresponding element

This is false, you are missing the break which stops the loop

This change can be an improvement, but the current code isn't wrong

However I don't think this will result in any improvement, as any decent optimizing compiler will optimize out this aspect on its own

Yes, my bad, I did miss the break keyword when checking this out. The reason I dig into the source code, is that when I rewrote this same code in my own function inside my script, the error went away. Will do more investigation.

@AThousandShips
Copy link
Member

The optimization might still be relevant, and to match the style of rfind, though

@MewPurPur
Copy link
Contributor

I don't see any harm, but it does look more like code style territory rather than performance.

@AThousandShips
Copy link
Member

No harm indeed, just a matter of style as optimization will 99.9% optimize this out and generate the equivalent code, just needs to be updates to clarify there's no bug here just an optimization/style upgrade

@Ivorforce
Copy link
Member

Ivorforce commented May 26, 2025

Hello, thank you again for opening this pull request!
I'm closing this PR now, since the changes have been superseded by recent find PRs, mostly #103932 and #104286. I'm happy to say the change you proposed made it into the refactor as well :)

Edit: I'm just now noticing that there are changes to array.cpp as well. As others above have pointed out, the change is unlikely to noticeably affect performance — but if you're still interested, feel free to rebase your branch and I'll reopen the PR.

@Ivorforce Ivorforce closed this May 26, 2025
@Ivorforce Ivorforce removed this from the 4.x milestone May 26, 2025
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