-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Hash#put_if_absent
#13590
Add Hash#put_if_absent
#13590
Conversation
is not |
|
src/spec/source.cr
Outdated
@@ -8,8 +8,7 @@ module Spec | |||
def self.read_line(file, line) | |||
return nil unless File.file?(file) | |||
|
|||
lines = lines_cache[file] ||= File.read_lines(file) | |||
lines[line - 1]? | |||
lines_cache.put_if_absent(file) { File.read_lines(file) }[line - 1]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local variable makes it more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean this?
lines_cache.put_if_absent(file) { File.read_lines(file) }[line - 1]? | |
lines = lines_cache.put_if_absent(file) { File.read_lines(file) } | |
lines[line - 1]? |
Then I agree =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once could argue that the .put_if_absent(file) { File.read_lines(file) } << item
constructs are bit overly complicated as well.
Too many API options. Why not keep it simple? What's a case where the existing way is too slow? |
A little benchmark shows some non-negligible improvement when keys are absent:
When keys aren't absent, then it's negligible. |
The performance impact depends on the hash complexity. But calculating the hash only once instead of twice is always a good improvement, even for simple keys. As #8660 (comment) shows, this is a very common API method for hash map implementations. Maybe not as concise as in Crystal but surely these languages have other ways to implement that with an added overhead of double hash calculation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer the above code suggestion, but I'm happy to merge with or without it.
Why is there no equivalent API in Ruby, I wonder? |
I can only speculate on that... Ruby's Anyway, regardless what Ruby is doing, this method seems to be very reasonable to me. Its optimization effect is basically a no-brainer. |
It is indeed proposed to Ruby in relatively recent times, called |
Resolves #8660.
This is faster than
hash[key] ||= value
for two reasons:Hash#insert_new
will not attempt to match the key against any existing keys at all. (This suggests thatfind_entry_with_index
followed byset_entry
orinsert_new
might be faster thanupsert
too, which makes#upsert
completely unnecessary, but I haven't verified that.)insert_new
, whereas||=
means any existing value must be upcast to a nilable value and evaluated for truthiness first.Instead of
#insert_new
, perhaps a copy of#upsert
that traverses existing keys but doesn't update them could be defined?Hash-like types such as
ENV.class
andURI::Params
do not have this. For those types it might be better if a generic implementation is offered via #10886.