RSpec/SubjectStub. Refactor and decrease complexity #925

andrykonchin · 2020-06-07T18:07:11Z

RSpec/SubjectStub takes about 35% of all the time spent in cops if run Rubocop on Rubocop's own source code.

PR addresses the performance problem mentioned in the rubocop/rubocop#8022 issue.

Profiler reports

Profiler report (stackprof) before optimizaition:

stackprof tmp/stackprof-cpu-rubocop.master.with-config.dump --method 'RuboCop::Cop::Commissioner#trigger_responding_cops'

RuboCop::Cop::Commissioner#trigger_responding_cops (/Users/andrykonchin/projects/rubocop/lib/rubocop/cop/commissioner.rb:50)
  samples:   756 self (2.8%)  /   17379 total (65.3%)
  callers:
    17379  (  100.0%)  block (2 levels) in <class:Commissioner>
    17332  (   99.7%)  RuboCop::Cop::Commissioner#trigger_responding_cops
    16630  (   95.7%)  RuboCop::Cop::Commissioner#with_cop_error_handling
  callees (16623 total):
    17332  (  104.3%)  RuboCop::Cop::Commissioner#trigger_responding_cops
    16652  (  100.2%)  RuboCop::Cop::Commissioner#with_cop_error_handling
    5934  (   35.7%)  RuboCop::Cop::RSpec::SubjectStub#on_block
    1998  (   12.0%)  RuboCop::Cop::Layout::SpaceAroundMethodCallOperator#on_send
     374  (    2.2%)  RuboCop::Cop::Layout::SpaceInsideArrayLiteralBrackets#on_array
...

Profiler report (stackprof) afeter optimizaition:

stackprof tmp/stackprof-cpu-rubocop.master.with-config.rspec-optimized.dump --method 'RuboCop::Cop::Commissioner#trigger_responding_cops'

RuboCop::Cop::Commissioner#trigger_responding_cops (/Users/andrykonchin/projects/rubocop/lib/rubocop/cop/commissioner.rb:50)
  samples:   851 self (3.6%)  /   13455 total (56.9%)
  callers:
    13455  (  100.0%)  block (2 levels) in <class:Commissioner>
    13398  (   99.6%)  RuboCop::Cop::Commissioner#trigger_responding_cops
    12598  (   93.6%)  RuboCop::Cop::Commissioner#with_cop_error_handling
  callees (12604 total):
    13398  (  106.3%)  RuboCop::Cop::Commissioner#trigger_responding_cops
    12626  (  100.2%)  RuboCop::Cop::Commissioner#with_cop_error_handling
    2143  (   17.0%)  RuboCop::Cop::Layout::SpaceAroundMethodCallOperator#on_send
     395  (    3.1%)  RuboCop::Cop::Layout::SpaceInsideArrayLiteralBrackets#on_array
     305  (    2.4%)  RuboCop::Cop::Layout::SpaceBeforeFirstArg#on_send
     279  (    2.2%)  RuboCop::Cop::Style::RedundantSelf#on_block
     266  (    2.1%)  RuboCop::RSpec::TopLevelDescribe#on_send
     226  (    1.8%)  RuboCop::Cop::Layout::SpaceInsideReferenceBrackets#on_send
     200  (    1.6%)  RuboCop::Cop::Style::TrailingMethodEndStatement#on_def
     200  (    1.6%)  RuboCop::Cop::Layout::FirstArgumentIndentation#on_send
     191  (    1.5%)  RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically#on_block
     175  (    1.4%)  RuboCop::Cop::Layout::SpaceAroundMethodCallOperator#on_const
     164  (    1.3%)  RuboCop::Cop::Layout::IndentationConsistency#on_begin
     136  (    1.1%)  RuboCop::Cop::Layout::SpaceInsideHashLiteralBraces#on_hash
     130  (    1.0%)  RuboCop::Cop::Layout::DefEndAlignment#on_send
     113  (    0.9%)  RuboCop::Cop::Interpolation#on_dstr
     109  (    0.9%)  RuboCop::Cop::RSpec::SubjectStub#on_block
...

Samples number decreased from 65.3% to 56.9% and the cop samples (relative to RuboCop::Cop::Commissioner#trigger_responding_cops method execution) decreased from 35.7% to 0.9%.

Total execution time

Before:

time bundle exec exe/rubocop --cache false --out rubocop.out .
bundle exec exe/rubocop --cache false --out rubocop.out .  83.46s user 0.40s system 99% cpu 1:24.40 total

After:

time bundle exec exe/rubocop --cache false --out rubocop.out .
bundle exec exe/rubocop --cache false --out rubocop.out .  62.60s user 0.35s system 99% cpu 1:03.21 total

Total execution time decreased from 1:24.40 to 1:03.21.

Rubocop version

In performance tests was used the following Rubocop master commit:

ommit 7bafa099902871d9e6dc91800390a5c32e8d086b (HEAD -> master)
Author: Bozhidar Batsov <bozhidar@batsov.com>
Date:   Sat May 30 14:05:28 2020 +0300

    [Docs] Fix a couple of references to the docs folder

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 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).

lib/rubocop/cop/rspec/subject_stub.rb

pirj · 2020-06-07T18:18:27Z

lib/rubocop/cop/rspec/subject_stub.rb

-          find_subject_stub(node) do |stub|
+          # skip already processed example group
+          # it's processed if is nested in one of the processed example groups
+          return unless (processed_example_groups & node.ancestors).empty?


How big are your specs so this makes a significant difference?

I ran Rubocop on its own source code so the test suite isn't large enough.

Regarding significance... This check avoids repeated analyzing of nested context/describe and I assume (I didn't test this TBH) it decrease the execution time by a factor of 2/3/4/... it depends on how many levels of nesting there are in the spec file.

I would prefer to avoid optimizations while sacrificing simplicity if we don't have enough proof that it gets us significant performance benefits. Also, long specs are hard to read and change. I'm not saying that we should deliberately make our tools slow to induce frustration when working with such specs, but primarily aiming to work with that kind of specs is not our aim.

Regarding the performance testing in the wild - you can pick some projects out of this list, I find https://github.com/discourse/discourse/ specifically good.

Checked this optimization on Gitlab source code and got the following results:

it speeds up the cope from 13% to 5.8% of samples

the whole execution time decreased by ~10s

Command to reproduce:

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

You can use on_top_level describe and then search for example groups, in order to avoid double lookups. E.g. once you are in an example group, you can break from going to nested example groups,

@Darhazer This is a truly amazing proposal.

But then it turns out that RuboCop::RSpec::TopLevelDescribe#on_send which triggers on_top_level_descrive, is one of the slowest methods

Ha, fair enough. Never looked closely at TopLevelDescribe. Frankly, I'm having a hard time understanding why it's implemented that way. Should be: "call on_top_level_describe if method name is :describe, and there are no ancestors of the type :block with a method name :describe". It'd better keep a set of already detected TLDs to quickly return from on_send if (node.ancestors & top_level_describes).any? just like Andrew has implemented in this pull request.

Luckily, it doesn't fit our purposes in this pull request anyway, since it's only for describe:

return false unless node.method_name == :describe

while we need something like TopLevelExampleGroup, probably implemented in a way I suggest above, but focusing on all example group methods, not only :describe.

Would you like to give this idea a shot in the scope of this pull request, or do you feel this improvement that you've done is good on its own already, @andrykonchin?

I would love to optimize TopLevelDescribe but think it deserves a separate PR.

Currently I am working on optimizing cops from the main rubocop gem but will definitely look at rubocop-rspec after that.

P.S.

But then it turns out that RuboCop::RSpec::TopLevelDescribe#on_send which triggers on_top_level_descrive, is one of the slowest methods

Switching from manual caching to #on_top_level_describe callback decreased profiler samples for SubjectStub cop from 0.9% to 0% so it isn't as bad as you may think 😉.

lib/rubocop/cop/rspec/subject_stub.rb

pirj · 2020-06-07T18:25:27Z

I guess if you rebase your code, the build will pass. Fix for those has been merged #914

pirj

I highly appreciate your effort of improving this cop, but please make correctness and simplicity your goals, not performance.

lib/rubocop/cop/rspec/subject_stub.rb

pirj · 2020-06-08T06:59:26Z

lib/rubocop/cop/rspec/subject_stub.rb

-          find_subject_stub(node) do |stub|
+          # skip already processed example group
+          # it's processed if is nested in one of the processed example groups
+          return unless (processed_example_groups & node.ancestors).empty?


I would prefer to avoid optimizations while sacrificing simplicity if we don't have enough proof that it gets us significant performance benefits. Also, long specs are hard to read and change. I'm not saying that we should deliberately make our tools slow to induce frustration when working with such specs, but primarily aiming to work with that kind of specs is not our aim.

lib/rubocop/cop/rspec/subject_stub.rb

pirj · 2020-06-08T07:22:34Z

Oh, sorry, I've completely missed your update in the PR description.

Total execution time decreased from 1:24.40 to 1:03.21.

This is truly impressive 👍
And it seems to be an incredibly slow cop.

…jects in example group

andrykonchin · 2020-06-08T13:02:27Z

@pirj Fixed all the issues. Please let me know to squash all the "code review" commits.

pirj · 2020-06-08T20:43:22Z

I have never thought we'll have to optimize anytime soon, but it is happening :D
Probably it's for the best.

… #on_block callback

andrykonchin · 2020-06-08T21:27:01Z

Addressed the issue with #on_top_leve_describe callback.

pirj · 2020-06-08T22:39:57Z

But it only detects subject stubbing inside a describe, does it?
On my current project, we use a top-level context.

lib/rubocop/cop/rspec/subject_stub.rb

Darhazer · 2020-06-09T07:53:38Z

lib/rubocop/cop/rspec/subject_stub.rb

+          node.each_descendant(:block).each_with_object({}) do |child, h|
+            name = subject(child)
+            if name
+              h[child.parent.parent] ||= []


what's the meaning of parent.parent? isn't it node.parent? while you are attaching to the parent?

parent should be a block node for outer describe/context/...

Will it work properly for contexts defined in iterators? (yes, another funny concept from my current project)

RSpec.describe do %i[one two].each do |count| context "for #{count}" do specify do expect(subject).to receive(count).and_return(0)

There's a parent block, but not an example group block.
I'm not sure this is the example to fail, hope you can figure out a better example to fail .parent.parent.

Darhazer · 2020-06-09T07:54:50Z

lib/rubocop/cop/rspec/subject_stub.rb

-          node.each_child_node do |child|
-            find_subject_expectation(child, subject_name, &block)
+        def find_subject_expectations(node, subject_names = [], &block)
+          if example_group?(node) && @explicit_subjects[node]


well I guess you can just check if there are explicit_subjects for this node, as if it is not an example group, there won't be any anyway. That could simplify the code

Darhazer · 2020-06-09T07:59:00Z

But it only detects subject stubbing inside a describe, does it?
On my current project, we use a top-level context.

Maybe you got the definition of top-level describe wrong. Isn't your spec like:

describe Something do
  context '...' do
     ...
  end
end

As that is a top-level describe, and I never saw RSpec tests that begin directly with a context. If there are, other cops may not work as well

andrykonchin · 2020-06-09T08:14:14Z

pirj
But it only detects subject stubbing inside a describe, does it?
On my current project, we use a top-level context.

@Darhazer
Agree with @pirj. There may be top-level shared examples etc... In addition there are some other similar methods like ~~xdebug~~ xdescribe. So I am going to revert the last commit.

Is there any similar helper to detect any top-level example group? Not only describe? Does it make sense to add it or it's OK to keep manual caching?

pirj · 2020-06-09T08:42:36Z

Maybe you got the definition of top-level describe wrong.

https://relishapp.com/rspec/rspec-core/docs/example-groups/basic-structure-describe - it surely hints that the basic structure is:

RSpec.describe "something" do
  context "in one context" do
    it "does one thing" do
    end
  end
end

But how about:

RSpec.shared_examples_for "it sabotages SubjectStub" do
  subject { haha }
  specify do
    expect(subject).to receive(:yes).and_return("no")
  end
end

We should detect this as well.
Is it a top-level example group? It doesn't really matter.

What about scenario, fdescribe, xdescribe? Why would we skip detection there?

As that is a top-level describe, and I never saw RSpec tests that begin directly with a context.

I've never seen it before as well. But it is how it is.

pirj · 2020-06-09T08:59:59Z

My point is not to limit SubjectStub to just describe, it's not how it worked before I believe.
A quick check shows that previously SubjectStub was detecting offences inside top-level xdescribe, fdescribe and feature.

Here's the default list. https://github.com/rspec/rspec-core/blob/2b913e1e36904227e3ecc8b4af0b7c622d3cb50d/lib/rspec/core/example_group.rb#L280
https://github.com/rspec/rspec-rails/blob/a9f90932d1939502d1024db12d99f5d413091bd4/lib/rspec/rails/example/feature_example_group.rb#L49 leaving us with:

example_group
describe
xdescribe
fdescribe
context
xcontext
fcontext
feature

For some of the cops, I don't see a good reason though to leave out those https://github.com/rspec/rspec-core/blob/ddbb43674be2a02c6cc839eba15f58265abcd8ca/lib/rspec/core/shared_example_group.rb#L100:

shared_examples
shared_examples_for
shared_context

…arching of outer example group

andrykonchin · 2020-06-09T23:18:21Z

Fixed all the mentioned issues. Please let me know to squash commits.

ping @pirj @Darhazer

pirj

Looks rock-solid. No changes in detection on 3500 files repo (~600 offences).
Before: 120s
After: 6.5s

Bravo! 👏

bquorning · 2020-06-11T08:36:56Z

Running rubocop --cache=false --only RSpec/SubjectStub -- spec to check about 1400 spec files. It took 73.19s before your change, 16.24s after. 👏

pirj · 2020-06-11T08:57:10Z

Thanks a lot @andrykonchin !

bquorning · 2020-06-11T09:06:34Z

I think this change warrants a mention in the Changelog.

For his incredible work on #925

For his incredible work on rubocop#925

Why? - it was slow #925 (comment) - it ignored non-describe top-level example groups #925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - #932 - #977

For his incredible work on rubocop/rubocop-rspec#925

Why? - it was slow rubocop/rubocop-rspec#925 (comment) - it ignored non-describe top-level example groups rubocop/rubocop-rspec#925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - rubocop/rubocop-rspec#932 - rubocop/rubocop-rspec#977

For his incredible work on rubocop/rubocop-rspec#925

Why? - it was slow rubocop/rubocop-rspec#925 (comment) - it ignored non-describe top-level example groups rubocop/rubocop-rspec#925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - rubocop/rubocop-rspec#932 - rubocop/rubocop-rspec#977

For his incredible work on rubocop/rubocop-rspec#925

Why? - it was slow rubocop/rubocop-rspec#925 (comment) - it ignored non-describe top-level example groups rubocop/rubocop-rspec#925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - rubocop/rubocop-rspec#932 - rubocop/rubocop-rspec#977

pirj marked this pull request as draft June 7, 2020 18:13

pirj added the refactor label Jun 7, 2020

pirj reviewed Jun 7, 2020

View reviewed changes

lib/rubocop/cop/rspec/subject_stub.rb Outdated Show resolved Hide resolved

pirj reviewed Jun 7, 2020

View reviewed changes

RSpec/SubjectStub. Refactor and decrease complexity

20e37fb

andrykonchin force-pushed the speedup-subject-stub-cop branch from 373acbe to 20e37fb Compare June 7, 2020 18:30

andrykonchin marked this pull request as ready for review June 7, 2020 21:08

andrykonchin requested a review from pirj June 7, 2020 21:09

RSpec/SubjectStub. Code review. Refactor #find_all_named_subjects method

817a77e

pirj reviewed Jun 8, 2020

View reviewed changes

andrykonchin marked this pull request as draft June 8, 2020 12:06

RSpec/SubjectStub. Code review. Handle case with several explicit sub…

7e76bd7

…jects in example group

andrykonchin requested a review from pirj June 8, 2020 13:01

andrykonchin marked this pull request as ready for review June 8, 2020 13:01

andrykonchin marked this pull request as draft June 8, 2020 19:32

pirj marked this pull request as ready for review June 8, 2020 20:43

RSpec/SubjectStub. Code review. Use #on_top_level_describe instead of…

341ae44

… #on_block callback

andrykonchin force-pushed the speedup-subject-stub-cop branch from 592f992 to 341ae44 Compare June 8, 2020 21:17

andrykonchin requested a review from Darhazer June 8, 2020 21:26

Darhazer reviewed Jun 9, 2020

View reviewed changes

RSpec/SubjectStub. Code review. Remove excessive condition and fix se…

956efea

…arching of outer example group

andrykonchin force-pushed the speedup-subject-stub-cop branch from d2430ae to 956efea Compare June 9, 2020 23:17

andrykonchin requested a review from Darhazer June 10, 2020 08:08

pirj approved these changes Jun 11, 2020

View reviewed changes

Darhazer approved these changes Jun 11, 2020

View reviewed changes

pirj merged commit 7f56f41 into rubocop:master Jun 11, 2020

pirj added a commit that referenced this pull request Jun 11, 2020

Add Andrew to the hall of fame

50535b5

For his incredible work on #925

pirj mentioned this pull request Jun 11, 2020

Add Andrew to the hall of fame #928

Merged

4 tasks

pirj added a commit that referenced this pull request Jun 11, 2020

Add Andrew to the hall of fame

686b1a8

For his incredible work on #925

andrykonchin deleted the speedup-subject-stub-cop branch June 11, 2020 12:18

andrykonchin mentioned this pull request Jun 12, 2020

RSpec/SubjectStub. Add top level group callback #932

Merged

6 tasks

mockdeep pushed a commit to mockdeep/rubocop-rspec that referenced this pull request Jun 14, 2020

Add Andrew to the hall of fame

d31e5bb

For his incredible work on rubocop#925

pirj mentioned this pull request Jul 22, 2020

Improve cops by using TopLevelGroup #977

Merged

6 tasks

pirj mentioned this pull request Jul 23, 2020

Retire TopLevelDescribe #978

Merged

7 tasks

pirj added a commit to rubocop/rubocop-capybara that referenced this pull request Dec 29, 2022

Add Andrew to the hall of fame

4fdbec0

For his incredible work on rubocop/rubocop-rspec#925

ydah pushed a commit to rubocop/rubocop-factory_bot that referenced this pull request Apr 13, 2023

Add Andrew to the hall of fame

d5625e1

For his incredible work on rubocop/rubocop-rspec#925

ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024

Add Andrew to the hall of fame

53581e8

For his incredible work on rubocop/rubocop-rspec#925

ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024

Add Andrew to the hall of fame

993f9ca

For his incredible work on rubocop/rubocop-rspec#925

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSpec/SubjectStub. Refactor and decrease complexity #925

RSpec/SubjectStub. Refactor and decrease complexity #925

andrykonchin commented Jun 7, 2020 •

edited

pirj Jun 7, 2020

andrykonchin Jun 7, 2020 •

edited

pirj Jun 8, 2020

pirj Jun 8, 2020

andrykonchin Jun 8, 2020 •

edited

Darhazer Jun 8, 2020

pirj Jun 8, 2020

Darhazer Jun 8, 2020

pirj Jun 8, 2020

andrykonchin Jun 8, 2020

pirj commented Jun 7, 2020

pirj left a comment

pirj Jun 8, 2020

pirj commented Jun 8, 2020

andrykonchin commented Jun 8, 2020

pirj commented Jun 8, 2020

andrykonchin commented Jun 8, 2020

pirj commented Jun 8, 2020

Darhazer Jun 9, 2020

andrykonchin Jun 9, 2020

pirj Jun 9, 2020

Darhazer Jun 9, 2020

Darhazer commented Jun 9, 2020

andrykonchin commented Jun 9, 2020 •

edited

pirj commented Jun 9, 2020 •

edited

pirj commented Jun 9, 2020

andrykonchin commented Jun 9, 2020 •

edited

pirj left a comment

bquorning commented Jun 11, 2020

pirj commented Jun 11, 2020

bquorning commented Jun 11, 2020

RSpec/SubjectStub. Refactor and decrease complexity #925

RSpec/SubjectStub. Refactor and decrease complexity #925

Conversation

andrykonchin commented Jun 7, 2020 • edited

Profiler reports

Total execution time

Rubocop version

Choose a reason for hiding this comment

andrykonchin Jun 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrykonchin Jun 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jun 7, 2020

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jun 8, 2020

andrykonchin commented Jun 8, 2020

pirj commented Jun 8, 2020

andrykonchin commented Jun 8, 2020

pirj commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darhazer commented Jun 9, 2020

andrykonchin commented Jun 9, 2020 • edited

pirj commented Jun 9, 2020 • edited

pirj commented Jun 9, 2020

andrykonchin commented Jun 9, 2020 • edited

pirj left a comment

Choose a reason for hiding this comment

bquorning commented Jun 11, 2020

pirj commented Jun 11, 2020

bquorning commented Jun 11, 2020

andrykonchin commented Jun 7, 2020 •

edited

andrykonchin Jun 7, 2020 •

edited

andrykonchin Jun 8, 2020 •

edited

andrykonchin commented Jun 9, 2020 •

edited

pirj commented Jun 9, 2020 •

edited

andrykonchin commented Jun 9, 2020 •

edited