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

Detect extra array allocation #6234

Merged
merged 1 commit into from Sep 7, 2018

Conversation

Projects
None yet
8 participants
@schneems
Contributor

schneems commented Aug 29, 2018

It is common to chain array methods for example

foo.compact.flatten.map { |x| x.downcase! }

Each of these methods compact, flatten, map will generate a new intermediate array that is promptly thrown away. Instead it is much faster to mutate when we know there’s a new array generated:

a = foo.compact
a.flatten!
a.map { |x| x.downcase! }
a

This PR adds a performance cop to detect this case and raise a failure.

Currently this cop does not support auto-correct and i’m not sure it ever can based on the amount of variety that the code changes may require.

This is my first attempt at writing a cop, so please consider this a rough draft. I need some help and guidance in terms of testing.

I’ve already run this against some major libraries and found quite a few optimizations.

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Aug 29, 2018

Contributor

BTW there's a ton of places in rubocop where extra arrays are being allocated. I started to fix them but it got to be too much and then I made the cop disabled by default which I intended to do anyway.

Contributor

schneems commented Aug 29, 2018

BTW there's a ton of places in rubocop where extra arrays are being allocated. I started to fix them but it got to be too much and then I made the cop disabled by default which I intended to do anyway.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Aug 30, 2018

Collaborator

Great first contribution! Overall it looks good to me, but I'll need to find some time to take a closer look.

It's a pity there isn't a better wait to chain mutating operations in Ruby - I guess that's the main reason why people normally don't pay much attention to this.

Collaborator

bbatsov commented Aug 30, 2018

Great first contribution! Overall it looks good to me, but I'll need to find some time to take a closer look.

It's a pity there isn't a better wait to chain mutating operations in Ruby - I guess that's the main reason why people normally don't pay much attention to this.

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Aug 30, 2018

Collaborator

👍

I made the cop disabled by default which I intended to do anyway.

I think this is a good idea for all the performance cops, as they tend to have at least some detriment to legibility.

Since RuboCop is not a long-running process, memory consumption isn't really much of a concern, but would be interesting to see some benchmark proving that this has a positive impact on execution time. 🙂

Collaborator

Drenmi commented Aug 30, 2018

👍

I made the cop disabled by default which I intended to do anyway.

I think this is a good idea for all the performance cops, as they tend to have at least some detriment to legibility.

Since RuboCop is not a long-running process, memory consumption isn't really much of a concern, but would be interesting to see some benchmark proving that this has a positive impact on execution time. 🙂

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Aug 30, 2018

Contributor

would be interesting to see some benchmark proving that this has a positive impact on execution time.

Here you go

require 'benchmark/ips'

array = %w[a b c d e f g h i j k l m n o p q r s t u v w x y z]
Benchmark.ips do |x| 
  x.report("mutate") {
    a = array.flatten
    a.compact!
    a
  }
  x.report("chain ") {
    array.flatten.compact
  }
  x.compare!
end
# Warming up --------------------------------------
#               mutate    30.529k i/100ms
#               chain     27.260k i/100ms
# Calculating -------------------------------------
#               mutate    337.642k (± 2.7%) i/s -      1.710M in   5.067136s
#               chain     292.043k (± 3.1%) i/s -      1.472M in   5.045385s

# Comparison:
#               mutate:   337642.4 i/s
#               chain :   292042.5 i/s - 1.16x  slower
Contributor

schneems commented Aug 30, 2018

would be interesting to see some benchmark proving that this has a positive impact on execution time.

Here you go

require 'benchmark/ips'

array = %w[a b c d e f g h i j k l m n o p q r s t u v w x y z]
Benchmark.ips do |x| 
  x.report("mutate") {
    a = array.flatten
    a.compact!
    a
  }
  x.report("chain ") {
    array.flatten.compact
  }
  x.compare!
end
# Warming up --------------------------------------
#               mutate    30.529k i/100ms
#               chain     27.260k i/100ms
# Calculating -------------------------------------
#               mutate    337.642k (± 2.7%) i/s -      1.710M in   5.067136s
#               chain     292.043k (± 3.1%) i/s -      1.472M in   5.045385s

# Comparison:
#               mutate:   337642.4 i/s
#               chain :   292042.5 i/s - 1.16x  slower
@calebthompson

This comment has been minimized.

Show comment
Hide comment
@calebthompson

calebthompson Aug 30, 2018

16% for one chained method is impressive. I imagine there are plenty of places where 4, 5, 6 methods are chained like this. If the pattern holds, that's a 112% improvement in speed.

calebthompson commented Aug 30, 2018

16% for one chained method is impressive. I imagine there are plenty of places where 4, 5, 6 methods are chained like this. If the pattern holds, that's a 112% improvement in speed.

@TheTeaNerd

This comment has been minimized.

Show comment
Hide comment
@TheTeaNerd

TheTeaNerd Aug 31, 2018

Perhaps it would be better to have just the new cop (& changelog etc) in this PR, and keep the applications of this new cop to Rubocop itself for a followup PR?

TheTeaNerd commented Aug 31, 2018

Perhaps it would be better to have just the new cop (& changelog etc) in this PR, and keep the applications of this new cop to Rubocop itself for a followup PR?

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 1, 2018

Collaborator

Yeah, I think that'd be best.

@schneems That's also going to eliminate the merge conflict we have right now.

Collaborator

bbatsov commented Sep 1, 2018

Yeah, I think that'd be best.

@schneems That's also going to eliminate the merge conflict we have right now.

@TheTeaNerd

This comment has been minimized.

Show comment
Hide comment
@TheTeaNerd

TheTeaNerd Sep 5, 2018

This cop was very helpful in highlighting some places where we were making and discarding several huge arrays without it being obvious, but I think there needs to be caution with the warning's wording as not all the foo! methods are consistent drop-in replacements, Even knowing that some of the bang methods return nil if no changes were made, I got caught out with a hard-to-figure-out bug.

I'm looking forward to seeing this in a future release!

TheTeaNerd commented Sep 5, 2018

This cop was very helpful in highlighting some places where we were making and discarding several huge arrays without it being obvious, but I think there needs to be caution with the warning's wording as not all the foo! methods are consistent drop-in replacements, Even knowing that some of the bang methods return nil if no changes were made, I got caught out with a hard-to-figure-out bug.

I'm looking forward to seeing this in a future release!

Detect extra array allocation
It is common to chain array methods for example

```
foo.compact.flatten.map { |x| x.downcase! }
```

Each of these methods `compact`, `flatten`, `map` will generate a new intermediate array that is promptly thrown away. Instead it is much faster to mutate when we know there’s a new array generated:

```
a = foo.compact
a.flatten!
a.map { |x| x.downcase! }
a
```

This PR adds a performance cop to detect this case and raise a failure.

Currently this cop does not support auto-correct and i’m not sure it ever can based on the amount of variety that the code changes may require.

This is my first attempt at writing a cop, so please consider this a rough draft. I need some help and guidance in terms of testing.

I’ve already run this against some major libraries and found quite a few optimizations.
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Sep 6, 2018

Contributor

@TheTeaNerd great! Any thoughts on wording for the cop output or docs that we can change to make that more clear?

Contributor

schneems commented Sep 6, 2018

@TheTeaNerd great! Any thoughts on wording for the cop output or docs that we can change to make that more clear?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Sep 6, 2018

Contributor

I removed all the conflicts. Let me know if there's anything else to be done here.

Contributor

schneems commented Sep 6, 2018

I removed all the conflicts. Let me know if there's anything else to be done here.

@bbatsov bbatsov merged commit 771d2ab into rubocop-hq:master Sep 7, 2018

17 checks passed

ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 7, 2018

Collaborator

Thanks for an excellent first contribution, @schneems! Keep those coming! ;-)

Collaborator

bbatsov commented Sep 7, 2018

Thanks for an excellent first contribution, @schneems! Keep those coming! ;-)

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Sep 11, 2018

Does anyone know how the performance of lazy enumerables compares to the suggested in-place mutation?

shepmaster commented Sep 11, 2018

Does anyone know how the performance of lazy enumerables compares to the suggested in-place mutation?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Sep 11, 2018

Contributor

@shepmaster like using the “lazy” followed by other enumerate methods? It’s much worse. Try out with my benchmark I posted earlier and modify to use the lazy method.

Contributor

schneems commented Sep 11, 2018

@shepmaster like using the “lazy” followed by other enumerate methods? It’s much worse. Try out with my benchmark I posted earlier and modify to use the lazy method.

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Sep 11, 2018

Indeed, that's a shame.

x.report("lazy  ") {
  array.lazy.flat_map{ |x| x }.reject{ |x| x.nil? }.force
}
Warming up --------------------------------------
              mutate    49.107k i/100ms
              chain     44.377k i/100ms
              lazy       5.363k i/100ms
Calculating -------------------------------------
              mutate    558.260k (± 3.7%) i/s -      2.799M in   5.021274s
              chain     506.103k (± 5.7%) i/s -      2.529M in   5.016187s
              lazy       54.306k (± 5.5%) i/s -    273.513k in   5.054780s

Comparison:
              mutate:   558260.3 i/s
              chain :   506102.6 i/s - 1.10x  slower
              lazy  :    54305.8 i/s - 10.28x  slower

shepmaster commented Sep 11, 2018

Indeed, that's a shame.

x.report("lazy  ") {
  array.lazy.flat_map{ |x| x }.reject{ |x| x.nil? }.force
}
Warming up --------------------------------------
              mutate    49.107k i/100ms
              chain     44.377k i/100ms
              lazy       5.363k i/100ms
Calculating -------------------------------------
              mutate    558.260k (± 3.7%) i/s -      2.799M in   5.021274s
              chain     506.103k (± 5.7%) i/s -      2.529M in   5.016187s
              lazy       54.306k (± 5.5%) i/s -    273.513k in   5.054780s

Comparison:
              mutate:   558260.3 i/s
              chain :   506102.6 i/s - 1.10x  slower
              lazy  :    54305.8 i/s - 10.28x  slower
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 11, 2018

Collaborator
Collaborator

bbatsov commented Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment