Add get_value_ref_or_add_default() to HashMap

[TokageItLab]

co-authored-by: @Ryan-000

• Split from Optimize animation performance and improve internals #112308

[Ivorforce]

Does adding this method make a big difference in practice for where this is used in the follow-up PRs? I'd be interested to see performance tests for this.
The alternative, as usual, is just using the getptr API, followed by an insertion if necessary. While slower, for most use-cases it suffices.

[Ryan-000]

This avoids hashing and looking up the same key twice by providing a single lookup-or-insert operation. This PR would likely be OK with the usual slower getptr() + insert pattern, but if we can get essentially free performance (and slightly cleaner code), it seems worth adding.

I’m basing this on C#’s CollectionsMarshal.GetValueRefOrAddDefault pattern which I use a lot; and I’ve noticed several places in Godot (e.g. Control::get_theme_*) where we do repeated lookups that would also benefit from this.

Eventually, when I have the time, I'll start submitting patches for them.

godot/scene/gui/control.cpp

Lines 3035 to 3043 in 2ecefad

	if (data.theme_icon_cache.has(p_theme_type) && data.theme_icon_cache[p_theme_type].has(p_name)) {
	return data.theme_icon_cache[p_theme_type][p_name];
	}
	Vector<StringName> theme_types;
	data.theme_owner->get_theme_type_dependencies(this, p_theme_type, theme_types);
	Ref<Texture2D> icon = data.theme_owner->get_theme_item_in_types(Theme::DATA_TYPE_ICON, p_name, theme_types);
	data.theme_icon_cache[p_theme_type][p_name] = icon;
	return icon;

[Ivorforce]

This avoids hashing and looking up the same key twice by providing a single lookup-or-insert operation. This PR would likely be OK with the usual slower getptr() + insert pattern, but if we can get essentially free performance (and slightly cleaner code), it seems worth adding.

No change is truly free. In this case, the change makes operator[] slightly more expensive, because of the additional argument and filling that argument. In addition, it makes the class slightly more complicated.

If this change should be merged, we need a better reason for it than "might as well do it" — we need a proof that it positively affects bottlenecks. Please refer to our optimization guidelines for the expectations of optimization PRs.

[Ryan-000]

If the performance of operator[] is a concern, I can keep its implementation as-is and add get_value_ref_or_add_default() separately, so only the new method carries any overhead from the extra argument.

There are many call sites that perform repeated hashing and lookups, which can be reduced. Here is an example from AnimationMixer.

godot/scene/animation/animation_mixer.cpp

Line 159 in 8105ff7

StringName key = lib.name == StringName() ? K.key : StringName(String(lib.name) + "/" + String(K.key));

We currently use has() and then operator[], which involves multiple lookups on both paths.

StringName key = /* some key */

if (!animation_set.has(key)) { // 1) Hash key and lookup.
    AnimationData ad;
    ad.animation = K.value;
    ad.animation_library = lib.name;
    ad.name = key;
    ad.last_update = animation_set_update_pass;
    animation_set.insert(ad.name, ad); // 2) Hash key and lookup again.
    cache_valid = false;
} else {
    AnimationData &ad = animation_set[key]; // 2) Hash key and lookup again.
    if (ad.last_update != animation_set_update_pass) {
        if (ad.animation != K.value || ad.animation_library != lib.name) {
            clear_cache_needed = true;
            ad.animation = K.value;
            ad.animation_library = lib.name;
        }

        ad.last_update = animation_set_update_pass;
    }
}

Faster getptr() + insert() approach, but still requires another hash and lookup on insertion path.

StringName key = /* some key */

AnimationData *ad = animation_set.getptr(key); // 1) Hash key and lookup
if (!ad) {
    ad = &animation_set.insert(key, AnimationData())->value; // 2) Hash key and lookup again.
    ad->animation = K.value;
    ad->animation_library = lib.name;
    ad->name = key;
    ad->last_update = animation_set_update_pass;
    cache_valid = false;
} else {
    if (ad->last_update != animation_set_update_pass) {
        if (ad->animation != K.value || ad->animation_library != lib.name) {
            clear_cache_needed = true;
            ad->animation = K.value;
            ad->animation_library = lib.name;
        }

        ad->last_update = animation_set_update_pass;
    }
}

Fastest, hash key once, lookup once.

StringName key = /* some key */

bool was_added = false;
AnimationData &ad = animation_set.get_value_ref_or_add_default(key, was_added); // Only 1 hash key and lookup.
if (was_added) {
    ad.animation = K.value;
    ad.animation_library = lib.name;
    ad.name = key;
    ad.last_update = animation_set_update_pass;
    cache_valid = false;
} else {
    if (ad.last_update != animation_set_update_pass) {
        if (ad.animation != K.value || ad.animation_library != lib.name) {
            clear_cache_needed = true;
            ad.animation = K.value;
            ad.animation_library = lib.name;
        }

        ad.last_update = animation_set_update_pass;
    }
}

The tradeoff is one additional method on AHashMap, in exchange for simpler call sites and fewer repeated lookups. It's a small change, and isn't currently on a hot path, but the reduction in work is still visible when profiling before vs after.

More broadly, much of the animation optimization work involved reducing unnecessary lookups, which is why I want to make this pattern easy to use going forward. Otherwise, I would have just used the getptr() + insert() approach above.

[Ivorforce]

If the performance of operator[] is a concern, I can keep its implementation as-is and add get_value_ref_or_add_default() separately, so only the new method carries any overhead from the extra argument.

That would introduce additional complexity to the class, and increase the chance of code cache misses.

The tradeoff is one additional method on AHashMap, in exchange for simpler call sites and fewer repeated lookups. It's a small change, and isn't currently on a hot path, but the reduction in work is still visible when profiling before vs after.

More broadly, much of the animation optimization work involved reducing unnecessary lookups, which is why I want to make this pattern easy to use going forward. Otherwise, I would have just used the getptr() + insert() approach above.

Sure, I understand the trade-off on a theoretical level.
But there are plenty of 'theoretical' performance problems in the engine (or rather, in all code) that we don't need to address, because it's not an actual problem. That's why our optimization guidelines ask for proofs of improvement for optimization PRs.

For example, insertion is quite rare (usually just during setup). So even if the getptr + insert approach makes an additional lookup on the insert path, it may not matter if it only happens once during setup.

[TokageItLab]

Furthermore, if this is truly effective, I believe it should be applied in areas beyond just Animation.