Performance. RSpec/LetSetup

[andrykonchin]

Optimize RSpec/LetSetup cop and #on_block callback.

Changes

• avoid excessive iteration over all the let! nodes nested into current example group
• fixed handling shared example groups

The approach is to iterate over only direct child let! nodes of a current example group.

Performance measurements

Timing for RuboCop::Cop::RSpec::LetSetup#on_block is changed from 10.2% to 1.3%.

Before

stackprof tmp/stackprof-cpu-gitlab.master.with-rubocop-rspec.LetSetup.dump --method 'RuboCop::Cop::RSpec::LetSetup#on_block'
RuboCop::Cop::RSpec::LetSetup#on_block (/Users/andrykonchin/projects/rubocop-rspec/lib/rubocop/cop/rspec/let_setup.rb:37)
  samples:     1 self (0.0%)  /   4003 total (10.2%)
  callers:
    4003  (  100.0%)  RuboCop::Cop::Commissioner#trigger_responding_cops
     144  (    3.6%)  RuboCop::Cop::RSpec::LetSetup#unused_let_bang
  callees (4002 total):
    3895  (   97.3%)  RuboCop::Cop::RSpec::LetSetup#unused_let_bang
     144  (    3.6%)  RuboCop::Cop::Cop#add_offense
     107  (    2.7%)  RuboCop::RSpec::Language::NodePattern#example_group?
  code:
                                  |    37  |           (block $(send nil? :let! (sym $_)) args ...)
  108    (0.3%) /     1   (0.0%)  |    38  |         PATTERN
                                  |    39  |
                                  |    40  |         def_node_search :method_called?, '(send nil? %)'
 3895    (9.9%)                   |    41  |
  144    (0.4%)                   |    42  |         def on_block(node)
                                  |    43  |           return unless example_group?(node)

After

stackprof tmp/stackprof-cpu-gitlab.master.with-rubocop-rspec.LetSetup.2.dump --method 'RuboCop::Cop::RSpec::LetSetup#on_block'
RuboCop::Cop::RSpec::LetSetup#on_block (/Users/andrykonchin/projects/rubocop-rspec/lib/rubocop/cop/rspec/let_setup.rb:42)
  samples:     1 self (0.0%)  /    440 total (1.3%)
  callers:
     440  (  100.0%)  RuboCop::Cop::Commissioner#trigger_responding_cops
     113  (   25.7%)  RuboCop::Cop::RSpec::LetSetup#unused_let_bang
  callees (439 total):
     361  (   82.2%)  RuboCop::Cop::RSpec::LetSetup#unused_let_bang
     113  (   25.7%)  RuboCop::Cop::Cop#add_offense
      78  (   17.8%)  RuboCop::RSpec::Language::NodePattern#example_group?
  code:
                                  |    42  |         def on_block(node)
   79    (0.2%) /     1   (0.0%)  |    43  |           return unless example_group?(node)
                                  |    44  |
                                  |    45  |           #puts node
  361    (1.1%)                   |    46  |           unused_let_bang(node) do |let|
  113    (0.3%)                   |    47  |             add_offense(let)
                                  |    48  |           end

Measurements approach

Used stackprof profiler to measure proportion of the cop timing. Running Rubocop on the GitLab project specs.

Run only one cope without caching and skip config with command

bundle exec exe/rubocop --cache false --out gitlab-specs.out --force-default-config  --require rubocop-rspec --only RSpec/LetSetup ../rubocop-profiling-examples/gitlabhq/spec

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

[pirj]

Those def_node_searches seem to be the root of all evil.

Thanks! Performance combo 🤜 👊 🤛 🤜

[Darhazer]

It would be great if you could yield if the passed block yielded, e.g. if there is shorter or more FP way to write the code below

[andrykonchin]

Do you mean something like this?

block_node.body.each_child_node(:block)
  .map { |child| let_bang(child) }
  .reject { |m| m.nil? }
  .each { |m| yield m }

[Darhazer]

yes, or

block_node.body.each_child_node(:block)
  .map(&method(:let_bang)
  .compact
  .each { |m| yield m }

But would prefer to have it even shorter, or at least without having to use explicit blocks
Ideally something like
block_node.body.each_child_node(:block).yield_when(&method(:let_bang)

Anyway, that was just a side note, as I hate to see things like

a = something
yield a if a

[pirj]

Sorry, I've missed that this has been discussed already.

[andrykonchin]

Compared generated offenses with master branch and found out some difference. Will investigate it so marked the PR as draft for now.

[andrykonchin]

Just realized the whole implementation of child_let_bang could be replaced with RuboCop::RSpec::ExampleGroup:

RuboCop::RSpec::ExampleGroup.new(node).lets.map { |let|  let_bang(let) }.compact

https://github.com/rubocop-hq/rubocop-rspec/blob/master/lib/rubocop/rspec/example_group.rb

I am wondering whether it makes sense to check this approach.

[pirj]

Absolutely. Those helpers keep being under the radar.

[andrykonchin]

Done. Looks pretty good.

[pirj]

Perfect! <3

[bquorning]

Good job. I added just a single comment.

[Darhazer]

🚀

[Darhazer]

Please squash the commits

[andrykonchin]

Done

[pirj]

Awesome perf-combo! 💯 Thanks a lot.