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

Add performance note to Array.resize() #84666

Merged

Conversation

MewPurPur
Copy link
Contributor

Adds a note about resizing ahead of time instead of adding new elements one by one. This would likely help some people trying to resolve performance bottlenecks.

@MewPurPur MewPurPur requested a review from a team as a code owner November 9, 2023 14:19
@MewPurPur MewPurPur force-pushed the document-resize-performance branch from e34047d to 926dac2 Compare November 9, 2023 14:20
@AThousandShips AThousandShips added this to the 4.x milestone Nov 9, 2023
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Other than some style details to decide on it looks good to me and relevant suggestion IMO (can be in 4.2 if consensus is reached)

@AThousandShips AThousandShips modified the milestones: 4.x, 4.3 Nov 9, 2023
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 9, 2023
@MewPurPur MewPurPur force-pushed the document-resize-performance branch from 926dac2 to 259880d Compare November 9, 2023 14:37
@arkology
Copy link
Contributor

arkology commented Nov 9, 2023

This is kinda obvious but ... Why not)

@AThousandShips
Copy link
Member

It isn't to many, people might not now how data is allocated and how expanding reallocates and might have to move all the data, this is far from obvious even to non-beginners

@MewPurPur
Copy link
Contributor Author

MewPurPur commented Nov 9, 2023

Should this note also be added to PackedArray classes? I haven't tested on those yet.

Edit: Doned.

@MewPurPur MewPurPur force-pushed the document-resize-performance branch from 259880d to 64f754a Compare November 9, 2023 15:04
@@ -544,6 +544,7 @@
<param index="0" name="size" type="int" />
<description>
Resizes the array to contain a different number of elements. If the array size is smaller, elements are cleared, if bigger, new elements are [code]null[/code]. Returns [constant OK] on success, or one of the other [enum Error] values if the operation failed.
Calling [method resize] and assigning the new values is faster than appending new elements one by one.
Copy link
Member

Choose a reason for hiding this comment

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

Note sure if such note is really helpful in the description of resize. If someone is not aware of this then they probably use append/push_back without checking the docs for resize. So it might make more sense to add some suggestion in descriptions of append/push_back instead.

Copy link
Contributor Author

@MewPurPur MewPurPur Nov 9, 2023

Choose a reason for hiding this comment

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

Fair point, though in my opinion it's not as practical because reasonably there are 4 places where the note is relevant. I see this note as a justification of why one would consider using resize(). If array operations are a bottleneck for your project, you'd likely look through all the array methods and see what you can do.

I'll change the "appending" wording to "adding", because it's also relevant for inserting and pushing to the front, even if not as much for obvious reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For clarity, I'm referring to: append() and push_back(), insert(), push_front().

Copy link
Member

Choose a reason for hiding this comment

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

It's not just for adding though, removing (erase, pop_at, pop_back, pop_front) also resizes (and hence can lead to reallocation).

And true, adding notes to all such methods makes little sense. Having it documented at least in on place (resize) probably does make sense (if we really assume it's not common knowledge).

Copy link
Contributor Author

@MewPurPur MewPurPur Nov 9, 2023

Choose a reason for hiding this comment

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

Personally, I only realized this because I saw performance-sensitive areas of Godot's code utilize it. Maybe it's obvious to people familiar with data structures, but we already address things I consider way more obvious in our Array docs (e.g. how adding elements at the beginning is slower because everything must be reindexed)

Also, as for these destructive methods, I don't think they count because calling resize() first causes a truncation?

Copy link
Member

Choose a reason for hiding this comment

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

Also, as for these destructive methods, I don't think they count because calling resize() first causes a truncation?

To be clear I wasn't saying that just calling resize() is enough for all of them. What should be used is case by case. Bulk pop_back is like bulk push_back/append, just a single resize() is equivalent/enough. For bulk insert, push_front, erase, pop_at, pop_back you'd also need some element shifting/manipulation before/after the resize(). What exactly depends on what's being done.

Anyway, I'm fine with the current notes. If we'd want to go into details then probably a whole separate manual page in the docs could be written about using/manipulating arrays. 🙃

Copy link
Member

@Calinou Calinou Nov 10, 2023

Choose a reason for hiding this comment

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

Anyway, I'm fine with the current notes. If we'd want to go into details then probably a whole separate manual page in the docs could be written about using/manipulating arrays.

This is probably a good idea to have, given there are many non-obvious things involving arrays:

  • Array vs typed array vs packed array (performance, flexibility, etc).
  • Shorthands for initializing packed arrays (implicit casting).
  • How to iterate multi-dimensional arrays effectively.

This information could also just be added to Data preferences.

@MewPurPur MewPurPur force-pushed the document-resize-performance branch from 64f754a to 80b6360 Compare November 9, 2023 22:41
@akien-mga akien-mga changed the title Add performance note to Array.resize() Add performance note to Array.resize() Jan 4, 2024
@akien-mga akien-mga merged commit 574e076 into godotengine:master Jan 4, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@MewPurPur MewPurPur deleted the document-resize-performance branch January 4, 2024 13:36
@YuriSizov YuriSizov removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

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.

7 participants