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.

Sign up for GitHub

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

Jump to bottom

Don't over use extend #8881

Closed
wants to merge 2 commits into from
Closed

Don't over use extend #8881

wants to merge 2 commits into from

Conversation

marcandre
Copy link
Contributor

The cop Style/MethodCallWithArgsParentheses does an extend at runtime in initialize.

This is not really needed in this case and this PR avoid that, while reducing the overall complexity and line count.

Other small changes were done in #7623 because of this; I didn't check if these should be reverted or not.

Note: To my surprise, I couldn't measure a difference performance-wise 😮

@marcandre marcandre mentioned this pull request Oct 10, 2020
Open
@marcandre marcandre mentioned this pull request Oct 10, 2020
Merged
marcandre added a commit that referenced this pull request Oct 10, 2020
@marcandre
Callback building was the single most time consuming method in all Ru…
79eabca 
…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 marcandre changed the title Dont over use extend Don't over use extend Oct 10, 2020
marcandre added a commit that referenced this pull request Oct 10, 2020
@marcandre
Callback building was the single most time consuming method in all Ru…
e1e12a9 
…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%)

```
@Drenmi
Copy link
Collaborator

Drenmi commented Oct 10, 2020

Wow. This is pretty wild! The cop was essentially some sort of factory pattern that used inheritance. 😅

I still think it would be nicer to use composition here, but I think this improvement is big enough to stand on its own.

marcandre added a commit that referenced this pull request Oct 16, 2020
@marcandre
Callback building was the single most time consuming method in all Ru…
cccbdca 
…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%)

```
mergify bot pushed a commit that referenced this pull request Oct 16, 2020
@marcandre @mergify
Callback building was the single most time consuming method in all Ru…
13df023 
…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
Copy link
Contributor Author

Merged

@marcandre marcandre closed this Oct 16, 2020
@marcandre marcandre deleted the dont_over_extend branch October 16, 2020 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcandre @Drenmi @bbatsov