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 const iteration support to Dictionary #104047

Merged
merged 1 commit into from
Mar 13, 2025

Conversation

YYF233333
Copy link
Contributor

Based on the assumption in #103999, we are able to bring ConstIterator to Dictionary without breaking the read_only property. Since the inner HashMap already has a nice iterator implementation, I just export it with alias to keep the change minimal.

Main usecase of this is to replace Dictionary::get_key_list(). Previously if we want to iterate a Dictionary we have to write:

List<Variant> keys;
dict.get_key_list(&keys);
for (const Variant &key : keys) {
    const Variant &value = dict[key];
    ......
}

This pattern includes unnecessary heap allocation (the List), redundant iteration (populate keys in get_key_list()) and an extra hashmap lookup (if you need both key and value).

With ConstIterator we can now write:

for (const KeyValue<Variant, Variant> &kv : dict) {
    const Variant &key = kv.key;
    const Variant &value = kv.value;
    ......
}

No heap allocation, scan exactly one time and no hashmap lookup.

I measure performance diff between the two pattern with MRP from #100333 (Note this workload is constructed intentionally to be bottlenecked at Dictionary iteration, Dictionary stringify would be a better showcase but unfortunately it is now bottlenecked by String concat)

Master:

729 ms
732 ms
729 ms
729 ms
727 ms
729 ms
725 ms
725 ms
720 ms
714 ms
average: 725 ms

This PR:

271 ms
270 ms
273 ms
271 ms
271 ms
272 ms
271 ms
273 ms
271 ms
272 ms
average: 271 ms

Callers who sort the key list or modify values in the loop are kept untouched.

Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Only needing a const iterator completely sidesteps the readonly hurdles, clever! I was a bit hesitant on exposing HashMap in the header, but we're already doing that with List so I suppose that can be dealt with when we start the clangd passes.

@Repiteo Repiteo added this to the 4.5 milestone Mar 12, 2025
@Repiteo Repiteo merged commit 22a7079 into godotengine:master Mar 13, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 13, 2025

Might be jumping the gun a bit here, but I'm confident enough about dictionaries to send this as-is.

Thanks!

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.

3 participants