so I thought: why not expose it to Dictionary

"Why not?" is never a good reason to add something to Core. While the code is minimal, it has no reason to be exposed to end users in the first place. It just adds another entry to the API that users will never touch. If sizing Dictionaries is a bottleneck, then you are already using the wrong data structure and no amount of calling reserve() is going to save you

This is exactly the sort of improvement contemplated by the first two of our contributor best practices:
https://contributing.godotengine.org/en/latest/engine/guidelines/best_practices.html#to-solve-the-problem-it-has-to-exist-in-the-first-place

"Why not?" is never a good reason to add something to Core.

You might have understood things wrong. Dictionary is used in countless places within the engine that can benefit from this.

While the code is minimal, it has no reason to be exposed to end users in the first place. It just adds another entry to the API that users will never touch.

Except it isn't? It's only exposed for internal use within the engine where beneficial.

My apologies if using the word "expose" led to wrong conclusions. I did a bit of rewording.

That's my bad. I didn't notice that there wasn't any binding code touched. That takes away my concern about this being exposed to end users. Adding something like this for internal use is a lot less of a problem.

That being said, if it's truly a benefit. Then this PR should be using it somewhere where dictionary allocations were a problem. Remember, the problem always needs to come first. We don't add solutions to the engine and then plan on finding problems. We start by identifying a problem, then we make changes to solve that problem.

This may very well be the correct solution to a problem, but in order to merge it, we need to know what that problem is.

It isn't enough to say that some places could benefit from this change. Show me what places do benefit from this change by profiling them before and after the change.

If sizing Dictionaries is a bottleneck, then you are already using the wrong data structure and no amount of calling reserve() is going to save you

That seems like a bit of a nonsensical take on this particular feature. reserve() is a standard feature for maps in most languages, and if you know the size AOT you can optimize with a reservation, which can be significant for large dictionaries or for manual copies or transformations of large dictionaries.

I would consider this a core library feature for Dictionary that is currently absent. Given that the cost of adding the API surface is effectively zero (as the API is a simple passthrough to the underlying HashMap), there would not be sufficient justification to deny it.

This may very well be the correct solution to a problem, but in order to merge it, we need to know what that problem is.

You do know what that problem is. Churn in a HashMap is wasteful. You don't need to have strict performance numbers for something that is this straightforward.

This may very well be the correct solution to a problem, but in order to merge it, we need to know what that problem is.

There are numerous situations within the engine where known final size dictionaries could be built faster for basically free by reserving. Isn't this a problem?

The point is that doing everything inside a single PR would be hard to keep track of.

Take #105928 for example, which does the same thing but for Vector, Array and String. It adds the possibility, and there are certainly more cases for that than this PR, but doesn't apply it anywhere.

The reason I did apply on the GDScript VM was mostly because that case was able to be reproduced and benchmarked within GDScript.

There are numerous situations within the engine where known final size dictionaries could be built faster for basically free by reserving. Isn't this a problem?

Sure, find one and show me. If there are numerous it should not be hard. I'm really not asking for much.

The point is that doing everything inside a single PR would be hard to keep track of.

Sure, don't do everything then. Just solve the low hanging fruit issues. As it stands this PR does nothing, I am asking for it to do something before merging.

That seems like a bit of a nonsensical take on this particular feature. reserve() is a standard feature for maps in most languages, and if you know the size AOT you can optimize with a reservation, which can be significant for large dictionaries or for manual copies or transformations of large dictionaries.

@Shadows-of-Fire Dictionary isn't our standard map in the engine, Hashmap is. By design we don't use Dictionary in any performance sensitive areas. Which is why I said above, if we find Dictionary in a performance sensitive areas, we are likely using the wrong data structure.

As I said above, if it's so obvious that this will fix a problem somewhere, then just find that problem and point it out. I'm not saying we shouldn't merge this, quite the contrary, I'm saying we should merge it after we have identified the problem it is solving and then used it to solve that problem.

If the problem is so obvious, you should have no problem finding it. To be blunt, don't waste your time arguing about whether something is useful in theory. I'm certain that 'reserve()' is useful in theory. Instead, spend your time showing where it is useful 'in practice'

A few cases I could find in core/, performance critical or not:

• Engine::get_version/author/copyright/donor/license_info()
• ProjectSettings::_add_builtin_input_map()
• Image::_get_data()
• marshalls.cpp (decode_variant())
• PListNode::get_value()
• ResourceLoaderBinary::parse_variant()
• ResourceFormatLoader::rename_dependencies()
• Resource::_duplicate_recursive()
• AStarGrid2D::get_point_data_in_region()
• TriangleMesh::intersect_*_scriptwrap()
• A few functions within Object
• Script::_get_script_constant_map() and ScriptServer::save_global_classes()
• OS::get_memory_info()
• Most functions in Time
• TranslationPO::_get_messages()
• Translation::_get_messages()

The one case I applied reserve() in this PR should speed up Dictionary construction from GDScript, as seen in my benchmark results. And no, we can't use HashMap there as it explicitly wants a Dictionary.

As it stands this PR does nothing

...

@clayjohn Growth is especially costly for hashing types, so reserve is especially useful. The PR currently optimizes the case of constructing dictionaries in GDScript, which sees a 8-15% gain from GDScript. This is already motivates the PR in its own right.

In general though, while we don't use Dictionary internally, we can expect this function to be useful when we have to construct Dictionary for the user (e.g. for function returns).
The main alternative would be offering copy / move constructors, which is generally better but not always feasible.

"Expose" is a very strong word in Godot, as you can see. I would suggest renaming the commit and title to something else. Maybe "Allow Dictionary to use reserve() in the GDScript VM"?

I'd say "Add reserve and use in GDScriptVM"

HashMap has a constructor where you can input the initial capacity and it will reserve right away. Would it be wise to add one to Dictionary too?

I'd keep that separately, that would only work for the C++ side due to overrides etc. so it's less critical, we don't have it for LocalVector either, it just saves one line

Rebased on top of #110717 changes.

I also noticed that Dictionary::recursive_duplicate() has a fill loop. I added a reserve there.

Thanks!