Performance. FactoryBot/AttributeDefinedStatically

[andrykonchin]

Optimized performance of FactoryBot/AttributeDefinedStatically cop.

Changes

• got rid of recursive def_node_search macros
• and avoid repeated processing the same block multiple times

Previous logic was

• take each block
• recursively find all the factory/trait/etc blocks in it
• and return all the defined attributes inside them

New logic

• take each block
• check whether it's factory/trait/etc
• return defined attributes

Performance measurements

Timing for RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block is changed from 6.8% to 0.3%.

Before

stackprof tmp/stackprof-cpu-gitlab.master.with-rubocop-rspec.AttributeDefinedStatically.dump --method 'RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block'
RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block (/Users/andrykonchin/projects/rubocop-rspec/lib/rubocop/cop/rspec/factory_bot/attribute_defined_statically.rb:38)
  samples:    21 self (0.1%)  /   1487 total (6.8%)
  callers:
    1487  (  100.0%)  RuboCop::Cop::Commissioner#trigger_responding_cops
       1  (    0.1%)  RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block
  callees (1466 total):
    1465  (   99.9%)  RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#factory_attributes
       1  (    0.1%)  RuboCop::AST::Node#receiver
       1  (    0.1%)  RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block
  code:
                                  |    38  |
 1487    (6.8%) /    21   (0.1%)  |    39  |           def_node_matcher :factory_attributes_matcher, <<-PATTERN
    1    (0.0%)                   |    40  |             (block (send _ #attribute_defining_method? ...) _ { (begin $...) $(send ...) } )
                                  |    41  |           PATTERN

Before

stackprof tmp/stackprof-cpu-gitlab.master.with-rubocop-rspec.AttributeDefinedStatically.2.dump --method 'RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block'
RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block (/Users/andrykonchin/projects/rubocop-rspec/lib/rubocop/cop/rspec/factory_bot/attribute_defined_statically.rb:43)
  samples:     6 self (0.0%)  /     57 total (0.3%)
  callers:
      57  (  100.0%)  RuboCop::Cop::Commissioner#trigger_responding_cops
  callees (51 total):
      51  (  100.0%)  RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#factory_attributes_matcher
  code:
                                  |    43  |           def on_block(node)
   51    (0.2%)                   |    44  |             attributes = factory_attributes_matcher(node) || []
    5    (0.0%) /     5   (0.0%)  |    45  |             attributes = [attributes] if !attributes.is_a?(Array)
                                  |    46  |
    1    (0.0%) /     1   (0.0%)  |    47  |             attributes.each do |attribute|
                                  |    48  |               next unless offensive_receiver?(attribute.receiver, node)

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 FactoryBot/AttributeDefinedStatically ../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]

Awesome!

It seems that node.to_a deconstructs by default, so an Array(node) doesn't work. I couldn't quickly find an elegant solution to this just from other cops' examples.

Thanks!

[bquorning]

Nitpick: I think these two lines could be written a bit simpler:

attributes = Array(factory_attributes(node))

[Darhazer]

I already suggested this, but unfortunately, node deconstructs itself in this case
#949 (comment)

[bquorning]

Ah, thanks for clarifying.