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 benchmark for Hash#dig vs #[] vs #fetch #102

Closed
wants to merge 3 commits into from

Conversation

@dideler
Copy link
Collaborator

@dideler dideler commented Mar 18, 2016

Comparison of how Ruby 2.3.0's Hash#dig performs to similar methods.

Unsafe retrieval options are included. I can remove them if you consider them noisy.

@Arcovion Arcovion added the Hash label Mar 18, 2016
end

x.report 'Hash#[] fallback' do
((((h[:a] || {})[:b] || {})[:c] || {})[:d] || {})[:e]
Copy link
Collaborator

@nateberkopec nateberkopec Mar 18, 2016

Choose a reason for hiding this comment

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

ouch, do people ever really do it this way? far more common I think is:

h[:a] && h[:a][:b] && ## etc

Copy link
Collaborator Author

@dideler dideler Mar 18, 2016

Choose a reason for hiding this comment

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

I'm not sure which way is more common. Should I use one example over the other or include both?

Choose a reason for hiding this comment

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

Actually, I think, the way proposed by @nateberkopec is more effective in case when none of the keys are exists, because you don't need to create all of these hashes.

Copy link
Collaborator Author

@dideler dideler Mar 18, 2016

Choose a reason for hiding this comment

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

I've added the more common style to the benchmark, see 95b8ea4

end

x.report 'Hash#fetch fallback' do
h.fetch(:a, {}).fetch(:b, {}).fetch(:c, {}).fetch(:d, {}).fetch(:e, nil)
Copy link
Collaborator

@ixti ixti Mar 18, 2016

Choose a reason for hiding this comment

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

fetch(:a, {}) creates new object EVERY time even when it's not needed.
More appropriate would be:

h.fetch(:a) { {} }.fetch(:b) { {} }...

Choose a reason for hiding this comment

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

Your notation will create Procs for every #fetch. That maybe faster or not. Seems that's another test… :)

Copy link
Collaborator Author

@dideler dideler Mar 19, 2016

Choose a reason for hiding this comment

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

@ixti I'd like to keep it as is because I see the current approach more often.

Copy link
Collaborator

@ixti ixti Mar 20, 2016

Choose a reason for hiding this comment

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

@mblumtritt in fact it will not. Why would it create Proc?

In any case, @dideler I tend to agree that probably some people use fetch(key, {}).
And that looks pretty fine, but in this case it worth to "cache" that object:

o = {}
h.fetch(:a, o).fetch(:b, o)

That's just my thoughts. I don't insist on anything :D

@ixti
Copy link
Collaborator

@ixti ixti commented Mar 18, 2016

More interesting would be to compare dig vs alternatives upon "broken" path:

H = { :a => { :b => { :d => true } } }

H.dig(:a, :b, :c, :d) # => nil
H[:a] && H[:a][:b] && H[:a][:b][:c] && H[:a][:b][:c][:d] # => nil

@dideler
Copy link
Collaborator Author

@dideler dideler commented Mar 20, 2016

More interesting would be to compare dig vs alternatives upon "broken" path:

I find the safe path most interesting because you can compare more options, but here are all cases.

Safe: h = { a: { b: { c: { d: { e: "foo" } } } } }

Comparison:
             Hash#[]:  6676415.9 i/s
            Hash#dig:  6215966.7 i/s - same-ish: difference falls within error
          Hash#[] ||:  6160177.6 i/s - same-ish: difference falls within error
          Hash#fetch:  4424551.0 i/s - 1.51x slower
 Hash#fetch fallback:  3278599.3 i/s - 2.04x slower
          Hash#[] &&:  3096090.4 i/s - 2.16x slower

Broken at last key: h = { a: { b: { c: { d: { x: "foo" } } } } }
The Hash#fetch benchmark is excluded because it doesn't gracefully fail.

Comparison:
             Hash#[]:  6391117.6 i/s
          Hash#[] ||:  5775857.7 i/s - same-ish: difference falls within error
            Hash#dig:  5599916.6 i/s - same-ish: difference falls within error
 Hash#fetch fallback:  3278820.7 i/s - 1.95x slower
          Hash#[] &&:  2920215.9 i/s - 2.19x slower

Broken at intermediate key: h = { a: { b: { c: { x: { e: "foo" } } } } }
The Hash#fetch and Hash#[] benchmarks are excluded because they do not gracefully fail.

Comparison:
            Hash#dig:  5525802.5 i/s
          Hash#[] ||:  4866123.4 i/s - same-ish: difference falls within error
          Hash#[] &&:  3727126.8 i/s - 1.48x slower
 Hash#fetch fallback:  2921706.4 i/s - 1.89x slower

Hash#dig does well in all cases. It reads better than the other options and is consistently performant.

@ixti
Copy link
Collaborator

@ixti ixti commented Mar 20, 2016

@dideler I'm absolutely agree that dig is a winner :D and I guess the only my proposal is to add link to #102 (comment) in readme :D

@dideler dideler force-pushed the hash-dig-vs-index-fetch branch from 73b2c8d to 437d8f8 Mar 20, 2016
@dideler
Copy link
Collaborator Author

@dideler dideler commented Mar 20, 2016

👍, how's 437d8f8?

@ixti
Copy link
Collaborator

@ixti ixti commented Mar 20, 2016

LGTM! 👍

@JuanitoFatas
Copy link
Owner

@JuanitoFatas JuanitoFatas commented Jun 16, 2016

Thanks everyone 👏 ! Merged as d6e1ac0.

@dideler Dennis would you like to be a collaborator to this repository 😊 ?

@dideler dideler deleted the hash-dig-vs-index-fetch branch Jun 16, 2016
@dideler
Copy link
Collaborator Author

@dideler dideler commented Jun 16, 2016

Thanks @JuanitoFatas - I would love to be a collaborator!

@JuanitoFatas
Copy link
Owner

@JuanitoFatas JuanitoFatas commented Jun 16, 2016

Thanks @JuanitoFatas - I would love to be a collaborator!

Yay! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants