Performance/HashEachMethods does not provide a net performance benefit

[urbanautomaton]

Background

The Performance/HashEachMethods cop was introduced in af84fb2, in PR #2578.

Since its introduction, there have been a large number of issues raised related to false positives (#5328, #5028, #4931, #4883, #4881, #4872, #4777, #4774, #4754, #4732).

While some of these have been addressed, the fundamental problem that the cop can't know what type of object is receiving the #each message means that most of these false positives will never be eliminated. The cop will match any invocation of #each yielding two arguments of which one is ignored: for example, iterating over an array of pairs.

As a workaround, the cop suggests wrapping destructuring block arguments in parens:

# violates the cop
[[1, 2], [3, 4]].each do |k, _|
  p k
end

# does not violate the cop
[[1, 2], [3, 4]].each do |(k, _)|
  p k
end

However, measurement suggests the cop's suggested changes do not reliably result in performance improvements, and that the suggested workaround for its false positives usually does have a negative performance impact.

Expected behavior

The Performance/HashEachMethods cop's suggested changes should improve code performance.

Actual behavior

The cop's suggested changes do not significantly improve performance in recent rubies, and the suggested workaround for its false positives significantly harms performance.

Steps to reproduce the problem

I ran a benchmark comparing Hash#each with Hash#each_key and Hash#each_value, and another comparing Array#each on an array of pairs, using implicit and explicit destructuring args.

The full benchmark code and results can be seen here, and are summarised below (for simplicity's sake I've looked only at key-only access, but the results for value-only are broadly similar).

Hash comparison (Hash#each = 1.0, larger number implies faster, measurements with the margin of error in italics):

Ruby:	2.2	2.3	2.4	2.5
Hash#each	1.0	1.0	1.0	1.0
Hash#each_key	1.16	1.15	1.02	0.99

Array comparison (|k, _| = 1.0, larger number implies faster, measurements within the margin of error in italics):

Ruby:	2.2	2.3	2.4	2.5
Array#each with k, _	1.0	1.0	1.0	1.0
Array#each with (k, _)	0.95	0.87	0.89	0.90

The small performance benefit to Hash#each_key that used to exist appears to have been eliminated in recent rubies, while a performance hit of similar magnitude remains when using parens for destructuring Array#each block arguments.

Conclusion

I think we should remove this cop, as:

• it no longer provides performance benefits in recent ruby versions,
• it does cost non-trivial end user time working around false positives, and
• it suggests a workaround for its false positives that is harmful to performance.

I'd be happy to write a PR for this if it's agreed we should do so.

RuboCop version

$ rubocop -V
0.52.1 (using Parser 2.4.0.2, running on ruby 2.4.3 x86_64-darwin17)

[rrosenblum]

Based on the performance numbers, I agree, this should be removed.