Performance. RSpec/NestedGroups #950
Conversation
andrykonchin
force-pushed
the
optimize-performance-nested-groups
branch
from
07b8610
to
12a45e7
Compare
andrykonchin
changed the title
|
Fixed all the issues |
andrykonchin
force-pushed
the
optimize-performance-nested-groups
branch
from
8a2a365
to
397ecb3
Compare
|
|
||
| def on_top_level_describe(node, _args) | ||
| find_nested_contexts(node.parent) do |context, nesting| | ||
| find_nested_example_groups(node.parent) do |example_group, nesting| | ||
| self.max = nesting |
There was a problem hiding this comment.
I wonder if this works correctly, e.g. if there are two example groups on the first level, and the first one has a larger number of nested groups, would the max be set to that number, or to the nesting level of the last offending group?
There was a problem hiding this comment.
It should work correctly as far as we don't track nesting level and calculate it every time:
def nesting_count(node)
count = node.each_ancestor(:block).count { |n| example_group?(n) }
count + 1
endThere was a problem hiding this comment.
Maybe I don't understand you well. What in your opinion may work incorrectly?
There was a problem hiding this comment.
Hmm, not sure how this self.max value is used but looks like setting a value smaller that previous one is handled correctly:
def max=(value)
cfg = config_to_allow_offenses
cfg[:exclude_limit] ||= {}
current_max = cfg[:exclude_limit][max_parameter_name]
value = [current_max, value].max if current_max # <====
cfg[:exclude_limit][max_parameter_name] = value
endhttps://github.com/rubocop-hq/rubocop/blob/master/lib/rubocop/cop/mixin/configurable_max.rb#L10-L16
There was a problem hiding this comment.
Yes, it's used when you run --auto-gen-config
Thank you for checking it though. I was going to verify myself, as it's not in the scope of the PR but I just noticed while reviewing the code
| def find_nested_example_groups(node, nesting: 1, &block) | ||
| yield node, nesting if example_group?(node) && nesting > max_nesting | ||
|
|
||
| next_nesting = example_group?(node) ? nesting + 1 : nesting |
There was a problem hiding this comment.
We seem to call example_group?(node) twice. WDYT of:
if example_group?(node)
yield node, nesting if nesting > max_nesting
nesting = nesting + 1
end
node.each_child_node do |child|
find_nested_example_groups(child, nesting: nesting, &block)
end| nested_context.each_child_node do |child| | ||
| find_nested_contexts(child, nesting: nesting + 1, &block) | ||
| end | ||
| node.each_child_node do |child| |
There was a problem hiding this comment.
It might be possible to squeeze more out of this with node.each_descendant(:block).
Which one would perform better?
There was a problem hiding this comment.
Filtering by node type looks reasonable (but each_descendant isn't suitable here because it's recursive but we need here direct children).
There is again one issue. We should handle not only block but at least begin node as well - if there are several nested contexts they are wrapped into begin node. TBH I am not sure whether there are another edge cases (e.g. kwbegin) or we could just filter block and begin nodes:
node.each_child_node(:block, :begin) do |child|There was a problem hiding this comment.
It has impressive impact - decreases the cop share from 3.3% to 1.8%.
There is no any difference between generated offenses with and without with filtering (checked on GitLab specs) but I still hesitate.
There was a problem hiding this comment.
I'm a bit lost. What change reduced the share from 3.3% to 1.8%?
There was a problem hiding this comment.
I meant additional filtering of block and begin nodes (what you've proposed) decreased cop time from 3.3% to 1.8%.
There was a problem hiding this comment.
ping @Darhazer @bquorning What do you think? Is it correct to filter nodes here?
There was a problem hiding this comment.
Yeah, I think using node.each_child_node(:block, :begin) is fine. If we are missing some edge cases, we can fix them as bugs. Provided of course that anyone will discover them.
There was a problem hiding this comment.
Thank you. Done.
andrykonchin
force-pushed
the
optimize-performance-nested-groups
branch
from
2af9a29
to
85b0399
Compare
|
Done |
|
Hey @andrykonchin. I just wanted to let you know that I tried running on the ~1400 spec files in my main work repository ( Time spent using rubocop-rspec v1.40.0: 1:20.71 total The time improvement is absolutely incredible, much better than I had hoped for. Thank you so much ❤️ |
|
Ahh, sorry. I was measuring the difference between 1.39.0 and master, not 1.40.0. But the speed improvement (probably in SubjectStub) is still your work. The runtime using v1.40.0 is “26.552 total” on my 1400 files. Still a significant improvement to master branch though. |
|
Yeah, |
|
I might be reading it wrong, does it appear twice? |
|
It's the way how stackprof shows different callers. |
|
Hmm... So looks like this statistic isn't aggregated like I thought before 😓 : |
Optimized performance of RSpec/NestedGroups and #on_top_level_describe callback.
Changes
Performance measurements
Timing for RSpec/NestedGroups is changed from 14.3% to 3.3% (for
RuboCop::RSpec::TopLevelDescribe#on_sendcallback).Before
After
Measurements approach
Used
stackprofprofiler 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
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).