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

Style/NumericPredicate -> defaults to slower code #3633

Closed
dennisvandehoef opened this issue Oct 16, 2016 · 5 comments
Closed

Style/NumericPredicate -> defaults to slower code #3633

dennisvandehoef opened this issue Oct 16, 2016 · 5 comments

Comments

@dennisvandehoef
Copy link

The default value (predicate) of the Style/NumericPredicate cop, produces slower code.
My opinion is that rubocop should default to the faster method, with the option of having the other layout.

Tested with Benchmark-ips & ruby 2.0, 2.1, 2.2, 2.3, 2.4dev

RuboCop::Cop::Style::NumericPredicate
  Benchmark
Warming up --------------------------------------
                == 0    94.390k i/100ms
               zero?    80.343k i/100ms
Calculating -------------------------------------
                == 0      2.498M (± 4.7%) i/s -     12.459M in   5.002912s
               zero?      1.804M (± 8.5%) i/s -      8.918M in   5.010642s

Comparison:
                == 0:  2498260.4 i/s
               zero?:  1803547.9 i/s - 1.39x  slower

    "== 0" is faster
Warming up --------------------------------------
                != 0    64.694k i/100ms
            nonzero?    49.316k i/100ms
Calculating -------------------------------------
                != 0      1.949M (±18.9%) i/s -      9.187M in   5.051024s
            nonzero?    895.233k (±26.2%) i/s -      4.093M in   5.066035s

Comparison:
                != 0:  1949177.3 i/s
            nonzero?:   895233.2 i/s - 2.18x  slower

    != 0" is faster
Warming up --------------------------------------
                 > 0    83.342k i/100ms
           positive?    73.005k i/100ms
Calculating -------------------------------------
                 > 0      2.629M (± 3.3%) i/s -     13.168M in   5.014540s
           positive?      1.567M (± 1.7%) i/s -      7.885M in   5.032639s

Comparison:
                 > 0:  2629417.0 i/s
           positive?:  1567175.2 i/s - 1.68x  slower

    "> 0" is faster
Warming up --------------------------------------
                 < 0    91.912k i/100ms
           negative?    82.470k i/100ms
Calculating -------------------------------------
                 < 0      2.328M (± 1.2%) i/s -     11.673M in   5.015670s
           negative?      1.952M (± 0.5%) i/s -      9.814M in   5.027162s

Comparison:
                 < 0:  2327595.1 i/s
           negative?:  1952235.9 i/s - 1.19x  slower

    "< 0" is faster

Test code example:

def self.slow_zero
  0.zero?
  1.zero?
  -1.zero?
  9558.zero?
  -9558.zero?
  1565156.zero?
  -1565156.zero?
end

def self.fast_zero
  0 == 0
  1 == 0
  -1 == 0
  9558 == 0
  -9558 == 0
  1565156 == 0
  -1565156 == 0
end
@mikegee
Copy link
Contributor

mikegee commented Oct 16, 2016

But is the performance difference significant? It looks like they are both very fast.

@dennisvandehoef
Copy link
Author

The result above has an average from about 62% slower.

But also the least result (1.19x slower) means that roughly i can do 5 times a < 0 and in the same time do 4 times a #negative?

@Drenmi
Copy link
Collaborator

Drenmi commented Oct 17, 2016

I would have to disagree with this. This is a style cop, not a performance cop. Robustness and readability (in this case readability is about the same) should be the guiding force for the defaults, not speculative micro-optimizations based on the current Ruby implementation.

Language level optimizations are a nebulous thing, potentially leading to us having different defaults per Ruby version, because of changes to the internals. (We recently removed a performance cop because the performance difference had been nuked somewhere around MRI 2.2.)

If someone does millions of comparisons to zero and, after proper, in vivo benchmarks, find that this has become a bottleneck, they can configure the cop or disable it for this block.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 20, 2016

I agree with @Drenmi.

@bbatsov bbatsov closed this as completed Oct 20, 2016
@dennisvandehoef
Copy link
Author

One comment I still have: @Drenmi sad "Language level optimizations" -> It is slower on ruby 2.0, 2.1, 2.2, 2.3 and 2.4

https://travis-ci.org/dennisvandehoef/rubocop-speed/builds/167869584

facebook-github-bot pushed a commit to facebook/chef-cookbooks that referenced this issue Oct 13, 2018
Summary:
This now lints for cases beyond just a comparison to 0, and encourage the use of .zero?. Even though we can configure this cop to lint and enforce a particular style (see [rubocop docs](https://rubocop.readthedocs.io/en/latest/cops_style/#stylenumericpredicate) for detail), the additional use cases below now mean that this cannot safely autocorrect. We can keep the autocorrect disabled (disabled by default) but since many of our users are more used to the conditional style and our codebase has several instances of the predicate style, we're choosing to disable for now.

It is worth noting that the conditional style is more performant, at least through ruby 2.4. rubocop/rubocop#3633

examples
```
EnforcedStyle: predicate (default)

foo == 0
0 > foo
bar.baz > 0

foo.zero?
foo.negative?
bar.baz.positive?
EnforcedStyle: comparison

foo.zero?
foo.negative?
bar.baz.positive?

foo == 0
0 > foo
bar.baz > 0
```

Reviewed By: jaymzh

Differential Revision: D10371724

fbshipit-source-id: 7ea7bfc3e365759e719355003448efcf167a3d9b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants