Add `Performance/Caller` cop #4078

alpaca-tc · 2017-02-28T10:32:46Z

Example

# bad
caller.first

# good
caller(1..1).first

Benchmark

require 'benchmark'

MAX = 10_000

def record(count = 100, &block)
  count.zero? ? yield : record(count - 1, &block)
end

def report(benchmark, name, &block)
  benchmark.report(name) do
    record { MAX.times(&block) }
  end
end

Benchmark.bm do |x|
  report(x, 'caller.first')       { caller.first }
  report(x, 'caller(1..1).first') { caller(1, 1).first }
end

#=>
#        user     system      total        real
# caller.first  1.470000   0.020000   1.490000 (  1.515402)
# caller(1..1).first  0.030000   0.010000   0.040000 (  0.035801)

Before submitting the PR make sure the following are checked:

Drenmi · 2017-02-28T15:23:50Z

config/enabled.yml

@@ -1342,6 +1342,11 @@ Lint/Void:

 #################### Performance ###########################

+Performance/CallerLocations:
+  Description: >-
+             Use `caller_locations` instance of `caller`


Did you mean instead of? 🙂

ooops... 😇

Drenmi · 2017-02-28T15:25:03Z

lib/rubocop/cop/performance/caller_locations.rb

+        SCOPE_METHODS = [:first, :[]].freeze
+
+        def on_send(node)
+          add_offense(node, node.loc.selector, MSG) if unneeded_caller?(node)


There should be a shortcut that allows you to do this:

add_offense(node, :selector)

Drenmi · 2017-02-28T15:27:32Z

spec/rubocop/cop/performance/caller_locations_spec.rb

+
+  it 'registers an offense when :[] is called on caller' do
+    inspect_source(cop, 'caller[0]')
+    expect(cop.messages).to eq([described_class::MSG])


We would normally assert:

expect(cop.offenses.size).to eq(1)

Asserting messages is generally only done for cops that have messages that change depending on context. 🙂

sachin21 · 2017-03-01T04:51:27Z

spec/rubocop/cop/performance/caller_locations_spec.rb

+
+require 'spec_helper'
+
+fdescribe RuboCop::Cop::Performance::CallerLocations do


fdescribe

This look like filter option.
If are you using filter option in development, you have to remove after development.

bbatsov · 2017-03-02T05:36:54Z

Some more background on this would be useful as the replacement code seems way more complex than the straightforward "slow" code.

alpaca-tc · 2017-03-04T08:36:49Z

@bbatsov I fixed to simply replace with caller(1..1) rather than complex caller_locations.

@dorian Thank you for reviewing 👍

Seems like it's fixable (lazy enumerator instead of Array, lazy hash, lazy array, etc) (function is vm_backtrace_to_ary).

hmm, could you tell me more details to what should i fix? 🤔

bbatsov · 2017-03-04T09:31:40Z

Just two more things:

in the first commit message drop the final .
in the second commit message - replace to with with

bbatsov · 2017-03-04T09:33:46Z

Btw, shouldn't this cop be more generic? If you do caller[n] it should be replaced with caller(n..n)? Seems a bit odd to optimize just for one case.

bbatsov · 2017-03-14T06:36:51Z

Any progress here? If not - I'll be forced to close this due to lack of activity.

alpaca-tc · 2017-03-14T14:14:11Z

Sorry, I kept you waiting. I am trying to improve this pr 🙏

rrosenblum · 2017-03-16T19:38:38Z

Just adding a bit more bench-marking information because I was curious.

[14] pry(main)> Benchmark.ips do |x|
[14] pry(main)*   x.report('caller.first') { caller.first }  
[14] pry(main)*   x.report('caller(1..1).first') { caller(1..1).first }  
[14] pry(main)*   x.compare!
[14] pry(main)* end  
Warming up --------------------------------------
        caller.first     2.314k i/100ms
  caller(1..1).first    36.661k i/100ms
Calculating -------------------------------------
        caller.first     24.241k (±13.5%) i/s -    120.328k in   5.055955s
  caller(1..1).first    490.166k (±16.5%) i/s -      2.383M in   5.028717s

Comparison:
  caller(1..1).first:   490166.1 i/s
        caller.first:    24240.7 i/s - 20.22x  slower

Most of the ideas for our performance cops came from fast-ruby. I didn't see this one listed. It might be nice to make a PR against their project as well.

alpaca-tc · 2017-03-20T11:58:23Z

@bbatsov Fixed 👍

sachin21 · 2017-03-21T14:38:06Z

@alpaca-tc You have to rebase and resolve conflicts.

bbatsov · 2017-03-23T06:07:50Z

The messages at least still mention 1 explicitly instead of being a generic index. Perhaps you forgot about them? I also don't see any tests for anything but 1.

rrosenblum · 2017-03-24T13:35:08Z

lib/rubocop/cop/performance/caller.rb

+          arguments = node.receiver.arguments
+
+          arguments.empty? ||
+            (arguments.length == 1 && arguments[0].type == :int)


Should this check arguments[0].int_type? instead of arguments[0].type == :int?

bbatsov · 2017-04-13T05:15:19Z

You should rebase this on top of the current master.

bbatsov · 2017-04-13T05:16:27Z

lib/rubocop/cop/performance/caller.rb

+      #   caller(n..n).first
+      #   caller(1..1).first
+      class Caller < Cop
+        MSG = 'Use `caller(n..n)` instead of `caller`.'.freeze


I guess instead of caller[n] would be more clear here.

aripollak · 2017-05-25T13:15:01Z

@alpaca-tc how come this suggests 1..1 instead of 1, 1? Either one has the same number of characters, but the former doesn't also require creating a Range object.

alpaca-tc · 2017-05-25T13:40:30Z

@aripollak yeah, it sounds nice

rrosenblum · 2017-05-25T20:29:03Z

I too noticed that the message could use some clarification. "Use caller(n..n) instead of caller[n]" isn't fully accurate. Wouldn't the correction need to be caller(n..n)[n] or caller(0, n)[n]?

alpaca-tc force-pushed the caller_locations branch 3 times, most recently from f21dbe2 to a16fe1c Compare February 28, 2017 11:35

alpaca-tc changed the title ~~[WIP] Add Performance/CallerLocations cop~~ Add Performance/CallerLocations cop Feb 28, 2017

Drenmi reviewed Feb 28, 2017

View reviewed changes

alpaca-tc force-pushed the caller_locations branch from a16fe1c to 98d1b92 Compare February 28, 2017 16:13

sachin21 reviewed Mar 1, 2017

View reviewed changes

alpaca-tc force-pushed the caller_locations branch 2 times, most recently from 567bc03 to 7f9969f Compare March 1, 2017 07:43

alpaca-tc force-pushed the caller_locations branch from 4d82fe4 to 017d495 Compare March 4, 2017 08:07

alpaca-tc changed the title ~~Add Performance/CallerLocations cop~~ Add Performance/Caller cop Mar 4, 2017

alpaca-tc force-pushed the caller_locations branch 4 times, most recently from f278d42 to c6ff058 Compare March 20, 2017 11:02

alpaca-tc force-pushed the caller_locations branch from c6ff058 to d719732 Compare March 22, 2017 07:42

rrosenblum reviewed Mar 24, 2017

View reviewed changes

alpaca-tc force-pushed the caller_locations branch from d719732 to d84889b Compare April 10, 2017 09:43

alpaca-tc force-pushed the caller_locations branch 3 times, most recently from 8eae5ab to 1499416 Compare April 11, 2017 01:25

bbatsov reviewed Apr 13, 2017

View reviewed changes

alpaca-tc force-pushed the caller_locations branch from 1499416 to 6ebd132 Compare April 15, 2017 14:54

alpaca-tc added 2 commits April 17, 2017 11:39

Replace caller.first with a faster syntax

210fb88

Add new Performance/Caller cop

f93009e

alpaca-tc force-pushed the caller_locations branch from 6ebd132 to f93009e Compare April 17, 2017 02:40

bbatsov merged commit e4e720b into rubocop:master Apr 17, 2017

alpaca-tc deleted the caller_locations branch April 18, 2017 12:07

ghost deleted a comment Jul 16, 2019

koppen mentioned this pull request Feb 7, 2020

Performance/Caller: Clearer message rubocop/rubocop-performance#99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Performance/Caller` cop #4078

Add `Performance/Caller` cop #4078

alpaca-tc commented Feb 28, 2017 •

edited

Loading

Drenmi Feb 28, 2017

alpaca-tc Feb 28, 2017

Drenmi Feb 28, 2017

Drenmi Feb 28, 2017

sachin21 Mar 1, 2017 •

edited

Loading

bbatsov commented Mar 2, 2017

alpaca-tc commented Mar 4, 2017

bbatsov commented Mar 4, 2017

bbatsov commented Mar 4, 2017

bbatsov commented Mar 14, 2017

alpaca-tc commented Mar 14, 2017 •

edited

Loading

rrosenblum commented Mar 16, 2017

alpaca-tc commented Mar 20, 2017

sachin21 commented Mar 21, 2017

bbatsov commented Mar 23, 2017

rrosenblum Mar 24, 2017

bbatsov Mar 24, 2017

bbatsov commented Apr 13, 2017

bbatsov Apr 13, 2017

aripollak commented May 25, 2017

alpaca-tc commented May 25, 2017

rrosenblum commented May 25, 2017 •

edited

Loading


		require 'spec_helper'

		fdescribe RuboCop::Cop::Performance::CallerLocations do

Add Performance/Caller cop #4078

Add Performance/Caller cop #4078

Conversation

alpaca-tc commented Feb 28, 2017 • edited Loading

Example

Benchmark

Drenmi Feb 28, 2017

Choose a reason for hiding this comment

alpaca-tc Feb 28, 2017

Choose a reason for hiding this comment

Drenmi Feb 28, 2017

Choose a reason for hiding this comment

Drenmi Feb 28, 2017

Choose a reason for hiding this comment

sachin21 Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

bbatsov commented Mar 2, 2017

alpaca-tc commented Mar 4, 2017

bbatsov commented Mar 4, 2017

bbatsov commented Mar 4, 2017

bbatsov commented Mar 14, 2017

alpaca-tc commented Mar 14, 2017 • edited Loading

rrosenblum commented Mar 16, 2017

alpaca-tc commented Mar 20, 2017

sachin21 commented Mar 21, 2017

bbatsov commented Mar 23, 2017

rrosenblum Mar 24, 2017

Choose a reason for hiding this comment

bbatsov Mar 24, 2017

Choose a reason for hiding this comment

bbatsov commented Apr 13, 2017

bbatsov Apr 13, 2017

Choose a reason for hiding this comment

aripollak commented May 25, 2017

alpaca-tc commented May 25, 2017

rrosenblum commented May 25, 2017 • edited Loading

Add `Performance/Caller` cop #4078

Add `Performance/Caller` cop #4078

alpaca-tc commented Feb 28, 2017 •

edited

Loading

sachin21 Mar 1, 2017 •

edited

Loading

alpaca-tc commented Mar 14, 2017 •

edited

Loading

rrosenblum commented May 25, 2017 •

edited

Loading