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

Optimize Hash#select(Enumerable) and #merge!(Hash, &) #12737

Merged

Conversation

HertzDevil
Copy link
Contributor

These two defs call has_key?(k) followed by [](k), performing a double lookup. This PR turns them into find_entry(k).

The optimization is quite noticeable when the keys are large structs, because their hash functions are more complex. I don't have a comprehensive benchmark but the speedup for #select is ~15% for UInt64 keys or ~60% for UInt8[128] keys.

@straight-shoota straight-shoota added this to the 1.7.0 milestone Nov 13, 2022
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @HertzDevil 🙏

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Nov 13, 2022

By the way there is no public API for this when the value type is nilable, i.e. when #[]? cannot distinguish a nil value from the absence of a key. The closest thing is find { |k, _| k == key }, but it has much poorer performance and doesn't account for @compare_by_identity and @block.

@asterite
Copy link
Member

Isn't that fetch with a block?

@HertzDevil
Copy link
Contributor Author

#fetch yields only when the key is not present. Here the desired behavior is to do something only when the key is present.

@straight-shoota straight-shoota merged commit ed21183 into crystal-lang:master Nov 14, 2022
@HertzDevil HertzDevil deleted the perf/hash-select-merge branch November 18, 2022 06:24
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.

None yet

4 participants