Skip to content

Commit

Permalink
Callback building was the single most time consuming method in all Ru…
Browse files Browse the repository at this point in the history
…boCop, at around 8% of overall processing.

This is because we currently build a new `Commissionner` for every file (could be optimized but not easy as config can change) and a commissioner must know which cops need responding to which node types.

Previous method was thus `O(files * cops * types)` where `types` is the number of node types encountered in a typical file.
Testing on `files == 200` typical RuboCop files, I calculated `types ~22.5`. Currently `cops == 422`.

This new algorithm is the same `O` but `types` is instead the number of types that a cop responds to (on average). For the cops that we run that is ~1.8.

The optimized cache building is almost 6x faster, for an *overall gain of ~6.6%*

It also has the advantage that adding new callbacks has zero impact on the cache building. Note: I'd like to add `on_after_send` etc (see #8844).

The only downside is that the list of callbacks is (by default) cached per Cop class.
This means that any Cop that somehow relies on *adding* callbacks it responds to *at runtime* is incompatible.

There's a single cop that does this (see #8881). Solutions include: not doing that (as in the PR), or only modifying the callbacks (in this case it could have been done by adding empty methods `on_send`, etc., even though they would be overriden by the extending modules), or overriding `Cop::Base#callbacks_needed` (although I marked the api as private for now).

How I tested performance:
```
$ stackprof tmp/stackprof.dump --text --method 'RuboCop::Cop::Commissioner#cops_callbacks_for'
  samples:  1039 self (7.5%)  /   1104 total (8.0%)

$ stackprof tmp/stackprof.dump --text --method 'RuboCop::Cop::Commissioner#initialize_callbacks'
  samples:    25 self (0.2%)  /    180 total (1.4%)

```
  • Loading branch information
marcandre committed Oct 16, 2020
1 parent 741c586 commit cccbdca
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 11 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,10 @@

* [#8892](https://github.com/rubocop-hq/rubocop/issues/8892): Fix an error for `Style/StringConcatenation` when correcting nested concatenable parts. ([@fatkodima][])

### Changes

* [#8882](https://github.com/rubocop-hq/rubocop/pull/8882): **(Potentially breaking)** RuboCop assumes that Cop classes do not define new `on_<type>` methods at runtime (e.g. via `extend` in `initialize`). ([@marcandre][])

## 0.93.1 (2020-10-12)

### Bug fixes
Expand Down
15 changes: 15 additions & 0 deletions lib/rubocop/cop/base.rb
Expand Up @@ -261,6 +261,21 @@ def offenses
'they are returned as the result of the investigation'
end

### Reserved for Commissioner

# @api private
def callbacks_needed
self.class.callbacks_needed
end

# @api private
def self.callbacks_needed
@callbacks_needed ||= public_instance_methods.select do |m|
m.match?(/^on_/) &&
!Base.method_defined?(m) # exclude standard "callbacks" like 'on_begin_investigation'
end
end

private

### Reserved for Cop::Cop
Expand Down
34 changes: 23 additions & 11 deletions lib/rubocop/cop/commissioner.rb
Expand Up @@ -45,8 +45,7 @@ def initialize(cops, forces = [], options = {})
@cops = cops
@forces = forces
@options = options
@callbacks = Hash.new { |h, k| h[k] = cops_callbacks_for(k) }
@restricted_map = {}
initialize_callbacks

reset
end
Expand Down Expand Up @@ -94,7 +93,7 @@ def investigate(processed_source)
private

def trigger_responding_cops(callback, node)
@callbacks[callback].each do |cop|
@callbacks[callback]&.each do |cop|
with_cop_error_handling(cop, node) do
cop.send(callback, node)
end
Expand All @@ -105,19 +104,32 @@ def reset
@errors = []
end

def cops_callbacks_for(callback)
callbacks = @cops.select do |cop|
cop.respond_to?(callback)
end
if RESTRICTED_CALLBACKS.include?(callback)
@restricted_map[callback] = restricted_map(callbacks)
def initialize_callbacks
@callbacks = build_callbacks(@cops)
@restricted_map = restrict_callbacks(@callbacks)
end

def build_callbacks(cops)
callbacks = {}
cops.each do |cop|
cop.callbacks_needed.each do |callback|
(callbacks[callback] ||= []) << cop
end
end
callbacks
end

def restrict_callbacks(callbacks)
restricted = {}
RESTRICTED_CALLBACKS.each do |callback|
restricted[callback] = restricted_map(callbacks[callback])
end
restricted
end

def trigger_restricted_cops(event, node)
name = node.method_name
@restricted_map.fetch(event)[name]&.each do |cop|
@restricted_map[event][name]&.each do |cop|
with_cop_error_handling(cop, node) do
cop.send(event, node)
end
Expand All @@ -127,7 +139,7 @@ def trigger_restricted_cops(event, node)
# Note: mutates `callbacks` in place
def restricted_map(callbacks)
map = {}
callbacks.select! do |cop|
callbacks&.select! do |cop|
restrictions = cop.class.send :restrict_on_send
restrictions.each do |name|
(map[name] ||= []) << cop
Expand Down

0 comments on commit cccbdca

Please sign in to comment.