Reduce and prevent unnecessary random-access to `List` #90705

AThousandShips · 2024-04-15T16:26:17Z

Random-access access to List when iterating is O(n^2) (O(n) when accessing a single element)

Removed subscript operator, in favor of a more explicit get
Added conversion from Iterator to ConstIterator
Remade existing operations into other solutions when applicable

Didn't (generally) replace uses of List with other structures except a few cases where it avoided a lot of code weirdness, it takes a lot more detailed evaluation of uses to handle that side, better left for other PRs by area maintainers

AThousandShips · 2024-04-15T16:27:44Z

core/doc_data.cpp

@@ -152,9 +152,10 @@ void DocData::method_doc_from_methodinfo(DocData::MethodDoc &p_method, const Met

 	return_doc_from_retinfo(p_method, p_methodinfo.return_val);

-	for (int i = 0; i < p_methodinfo.arguments.size(); i++) {
+	int i = 0;


These are to allow the use of the i variable, and unfortunately c++17 doesn't allow initializing multiple variables of different types in the for loop

AThousandShips · 2024-04-15T16:28:02Z

core/extension/extension_api_dump.cpp

-						Array arguments;
-						for (int i = (has_return ? -1 : 0); i < mi.arguments.size(); i++) {
-							PropertyInfo pinfo = i == -1 ? mi.return_val : mi.arguments[i];
+						if (has_return) {


Split this up because of the use of iterators

editor/plugins/version_control_editor_plugin.cpp

main/main.cpp

lawnjelly · 2024-05-04T13:46:36Z

modules/gdscript/gdscript_compiler.cpp

-		const PropertyInfo &prop = info.arguments[i];
-		if (!_is_exact_type(prop, p_arguments[i].type)) {
+	int i = 0;
+	for (List<PropertyInfo>::ConstIterator itr = info.arguments.begin(); itr != info.arguments.end(); ++itr, ++i) {


There's a few of these cases, seems a common paradigm.

I do wonder if using the for each simpler version and putting the i++ on the first line might end up being easier to read. Maybe there's a more elegant way.

Can look into them

I did these, as seen above, to avoid any risky cases to ensure the iteration is clean, keeping the iteration in one place feels safer to me given some errors I accidentally caused with some of these cases when I used for each syntax, so will keep these for now for a potential cleanup down the line to ensure they work correctly

platform/windows/display_server_windows.cpp

Random-access access to `List` when iterating is `O(n^2)` (`O(n)` when accessing a single element) * Removed subscript operator, in favor of a more explicit `get` * Added conversion from `Iterator` to `ConstIterator` * Remade existing operations into other solutions when applicable

lawnjelly · 2024-05-04T14:09:49Z

Overall this looks great.

I'd be tempted to take the plunge and use local variables (e.g. reference) in a few cases where multiple front()->get() etc are used, just in order to reduce the verbosity.

These weren't there originally granted, so I understand being wary of introducing bugs via duplicate variable names or something. Or could be left to another PR.

AThousandShips · 2024-05-04T14:10:50Z

Fixed the unnecessary referencing in main, but left most of the other cases alone, as it's a bit of a larger careful refactor, and can do those in a follow-up

lawnjelly

Looks okay from looking through as best I could (quite long PR), although not tested.

Calinou · 2024-05-06T23:17:21Z

I've tried to compare results between master and this PR on https://github.com/godotengine/godot-benchmarks, but the benchmarks where it would be the most relevant tend to have high runtime variations.

Either way, I've gotten this from hyperfine by running all its GDScript benchmarks:

❯ hyperfine 'bin/godot.linuxbsd.editor.x86_64.master --path ~/Documents/Git/godotengine/godot-benchmarks -- --run-benchmarks --include-benchmarks="*gdscript*"' 'bin/godot.linuxbsd.editor.x86_64 --path ~/Documents/Git/godotengine/godot-benchmarks -- --run-benchmarks --include-benchmarks="*gdscript*"'              
Benchmark 1: bin/godot.linuxbsd.editor.x86_64.master --path ~/Documents/Git/godotengine/godot-benchmarks -- --run-benchmarks --include-benchmarks="*gdscript*"
  Time (mean ± σ):     41.086 s ±  0.164 s    [User: 39.285 s, System: 0.795 s]
  Range (min … max):   40.770 s … 41.294 s    10 runs
 
Benchmark 2: bin/godot.linuxbsd.editor.x86_64 --path ~/Documents/Git/godotengine/godot-benchmarks -- --run-benchmarks --include-benchmarks="*gdscript*"
  Time (mean ± σ):     41.053 s ±  0.176 s    [User: 39.270 s, System: 0.777 s]
  Range (min … max):   40.801 s … 41.325 s    10 runs
 
Summary
  bin/godot.linuxbsd.editor.x86_64 --path ~/Documents/Git/godotengine/godot-benchmarks -- --run-benchmarks --include-benchmarks="*gdscript*" ran
    1.00 ± 0.01 times faster than bin/godot.linuxbsd.editor.x86_64.master --path ~/Documents/Git/godotengine/godot-benchmarks -- --run-benchmarks --include-benchmarks="*gdscript*"

This PR seems to provide a tiny overall speedup (in terms of overall runtime, including startup + shutdown) across an average of 10 runs for each. Standard deviation is still quite high so take it with a grain of salt, but at least we can be assured this change doesn't harm performance.

lawnjelly · 2024-05-07T07:00:58Z

This PR seems to provide a tiny overall speedup (in terms of overall runtime, including startup + shutdown) across an average of 10 runs for each. Standard deviation is still quite high so take it with a grain of salt, but at least we can be assured this change doesn't harm performance.

It is good to confirm there is no slowdown. 👍
In terms of performance increases though, bear in mind the primary intended effect here is protective.

Existing common path performance sapping cases will (hopefully) mostly already have been fixed. The aim here is to make it more difficult to inadvertently introduce performance hotspots in the future, which then need extra work to identify and fix.

There's a number of other data structures which tend to be more appropriate if elements are heavy, there is random insertions / deletions / moves, and indexed access is required, which is why indexed access to linked list is usually a bit of a "code smell".

akien-mga · 2024-05-07T07:07:03Z

Thanks!

AThousandShips · 2024-05-07T07:34:45Z

Thank you!

If you or anyone come across things related to this just point me that direction if any help is needed for adjusting to these changes

AThousandShips · 2024-05-07T07:37:07Z

In terms of performance increases though, bear in mind the primary intended effect here is protective.

Exactly, thank you

I am going to add a few follow-up improvements, like replacements with LocalVector, also gonna open a PR with a get_front/back now that this is merged, but some areas are more appropriate for area maintainers to look at for replacements, but they'll be much easier to identify now given the dedicated syntax

aaronfranke · 2024-05-16T11:34:41Z

Removed subscript operator, in favor of a more explicit get

What is the replacement for setting at a given index?

AThousandShips · 2024-05-16T12:11:51Z

Using foo.get(i) = x;, I chose not to add an explicit set for clutter, can do if it's needed for clarity, but the method is a drop in replacement for the operator

Bur you likely shouldn't be using List if you need to do this

aaronfranke · 2024-05-16T12:21:10Z

I'm passing data to OS::get_singleton()->execute, which uses List. I have to use List because this is Godot's API.

AThousandShips · 2024-05-16T12:22:46Z

I think editing the elements in this way isn't necessary, adding and removing from the end, or creating a new list might be better

AThousandShips added enhancement topic:core performance labels Apr 15, 2024

AThousandShips added this to the 4.x milestone Apr 15, 2024

AThousandShips requested review from a team as code owners April 15, 2024 16:26

AThousandShips commented Apr 15, 2024

View reviewed changes

lawnjelly reviewed May 4, 2024

View reviewed changes

editor/plugins/version_control_editor_plugin.cpp Outdated Show resolved Hide resolved

lawnjelly reviewed May 4, 2024

View reviewed changes

main/main.cpp Show resolved Hide resolved

lawnjelly reviewed May 4, 2024

View reviewed changes

platform/windows/display_server_windows.cpp Outdated Show resolved Hide resolved

AThousandShips force-pushed the foreach_list branch from 9a09918 to 955d5af Compare May 4, 2024 14:09

lawnjelly approved these changes May 4, 2024

View reviewed changes

akien-mga approved these changes May 7, 2024

View reviewed changes

akien-mga modified the milestones: 4.x, 4.3 May 7, 2024

akien-mga merged commit e63252b into godotengine:master May 7, 2024
16 checks passed

akien-mga mentioned this pull request May 7, 2024

Generate docs from GDExtensions using --gdextension-docs with --doctool #91518

Merged

AThousandShips deleted the foreach_list branch May 7, 2024 07:33

AThousandShips mentioned this pull request May 7, 2024

[Core] Add convenience access to front/back element of List #91649

Open

This comment was marked as outdated.

Sign in to view

This was referenced May 7, 2024

[Core] Reduce and prevent unnecessary random-access to List godotengine/godot-cpp#1457

Merged

[Core] Use Vector for MethodInfo::arguments #91660

Open

jsjtxietian mentioned this pull request May 10, 2024

Consider Enhancing the Performance Transparency of List's Operator[] #89939

Closed

RadiantUwU mentioned this pull request Aug 23, 2024

Crash when debugging with neovim from a use-after-free with List #95998

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce and prevent unnecessary random-access to `List` #90705

Reduce and prevent unnecessary random-access to `List` #90705

AThousandShips commented Apr 15, 2024 •

edited

Loading

AThousandShips Apr 15, 2024

AThousandShips Apr 15, 2024

lawnjelly May 4, 2024

AThousandShips May 4, 2024

AThousandShips May 4, 2024

lawnjelly commented May 4, 2024

AThousandShips commented May 4, 2024

lawnjelly left a comment

Calinou commented May 6, 2024 •

edited

Loading

lawnjelly commented May 7, 2024

akien-mga commented May 7, 2024

AThousandShips commented May 7, 2024

AThousandShips commented May 7, 2024

This comment was marked as outdated.

aaronfranke commented May 16, 2024 •

edited

Loading

AThousandShips commented May 16, 2024 •

edited

Loading

aaronfranke commented May 16, 2024

AThousandShips commented May 16, 2024

Reduce and prevent unnecessary random-access to List #90705

Reduce and prevent unnecessary random-access to List #90705

Conversation

AThousandShips commented Apr 15, 2024 • edited Loading

AThousandShips Apr 15, 2024

Choose a reason for hiding this comment

AThousandShips Apr 15, 2024

Choose a reason for hiding this comment

lawnjelly May 4, 2024

Choose a reason for hiding this comment

AThousandShips May 4, 2024

Choose a reason for hiding this comment

AThousandShips May 4, 2024

Choose a reason for hiding this comment

lawnjelly commented May 4, 2024

AThousandShips commented May 4, 2024

lawnjelly left a comment

Choose a reason for hiding this comment

Calinou commented May 6, 2024 • edited Loading

lawnjelly commented May 7, 2024

akien-mga commented May 7, 2024

AThousandShips commented May 7, 2024

AThousandShips commented May 7, 2024

This comment was marked as outdated.

aaronfranke commented May 16, 2024 • edited Loading

AThousandShips commented May 16, 2024 • edited Loading

aaronfranke commented May 16, 2024

AThousandShips commented May 16, 2024

Reduce and prevent unnecessary random-access to `List` #90705

Reduce and prevent unnecessary random-access to `List` #90705

AThousandShips commented Apr 15, 2024 •

edited

Loading

Calinou commented May 6, 2024 •

edited

Loading

aaronfranke commented May 16, 2024 •

edited

Loading

AThousandShips commented May 16, 2024 •

edited

Loading