Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Commissioner callbacks #8882

Merged
merged 4 commits into from Oct 16, 2020
Merged

Optimize Commissioner callbacks #8882

merged 4 commits into from Oct 16, 2020

Commits on Oct 16, 2020

  1. Remove unused method

    marcandre committed Oct 16, 2020
    Copy the full SHA
    f022bb1 View commit details
    Browse the repository at this point in the history
  2. Copy the full SHA
    0124131 View commit details
    Browse the repository at this point in the history
  3. Optimize Cop#restrict_on_send

    We only use the result to iterate, so an array is better here
    marcandre committed Oct 16, 2020
    Copy the full SHA
    741c586 View commit details
    Browse the repository at this point in the history
  4. Callback building was the single most time consuming method in all Ru…

    …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%)
    
    ```
    marcandre committed Oct 16, 2020
    Copy the full SHA
    cccbdca View commit details
    Browse the repository at this point in the history