Turn on performance based cops #32381

dillonwelch · 2018-03-30T02:42:01Z

Summary

Use attr_reader/attr_writer instead of methods

method is 12% slower

Use flat_map over map.flatten(1)

flatten is 66% slower

Use hash[]= instead of hash.merge! with single arguments

merge! is 166% slower

See #32337 for more conversation

rails-bot · 2018-03-30T02:42:03Z

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

dillonwelch · 2018-03-30T05:50:13Z

For the remaining Rubocop errors, one of

They didn't seem to meet standard Rails styles (the t.timestamps especially)
The existing code behavior was directly called out in the test description (header_test.rb)
I would have to do a weird block to define instance methods for attr_reader
The change doesn't actually work (relation_test.rb)

Any thoughts on whether to leave these, add ignore comments, or ideas on how to fix?

qinmingyuan · 2018-03-30T10:47:11Z

.rubocop.yml

@@ -160,3 +160,15 @@ Style/Semicolon:
 # Prefer Foo.method over Foo::method
 Style/ColonMethodCall:
  Enabled: true
+
+Style/SymbolProc:
+  Enabled: true


Prefer Non Symbol Proc, I think { |timer| timer.shutdown } it is more readable than (&:shutdown)

I agree, take this out unless it's also faster. Let's scope this change to only performance.

Benchmark/ips says that they're about the same

require 'benchmark/ips' array = ["foo", "bar", "baz"] Benchmark.ips do |x| x.report("symbol proc ") { array.each(&:upcase) } x.report("regular proc") { array.each {|x| x.upcase } } x.compare! end

schneems

Overall I like the idea. We should remove the Style/SymbolProc: as it doesn't appear to be faster and makes this change much more noisy.

Also looks like there's some code climate failures.

schneems · 2018-03-30T17:02:07Z

.rubocop.yml

@@ -160,3 +160,15 @@ Style/Semicolon:
 # Prefer Foo.method over Foo::method
 Style/ColonMethodCall:
  Enabled: true
+
+Style/SymbolProc:
+  Enabled: true


I agree, take this out unless it's also faster. Let's scope this change to only performance.

schneems · 2018-03-30T17:05:07Z

.rubocop.yml

@@ -160,3 +160,15 @@ Style/Semicolon:
 # Prefer Foo.method over Foo::method
 Style/ColonMethodCall:
  Enabled: true
+
+Style/SymbolProc:
+  Enabled: true


Benchmark/ips says that they're about the same

require 'benchmark/ips' array = ["foo", "bar", "baz"] Benchmark.ips do |x| x.report("symbol proc ") { array.each(&:upcase) } x.report("regular proc") { array.each {|x| x.upcase } } x.compare! end

schneems · 2018-03-30T17:07:47Z

.rubocop.yml

+Style/SymbolProc:
+  Enabled: true
+
+Style/TrivialAccessors:


While it's labeled as "style" this is indeed faster

require 'benchmark/ips' class Foo attr_accessor :a def initialize @a = "hello" @b = "world" end def b @b end end foo = Foo.new Benchmark.ips do |x| x.report("attr") { foo.a } x.report("def ") { foo.b } x.compare! end

gives

Warming up -------------------------------------- attr 338.812k i/100ms def 328.549k i/100ms Calculating ------------------------------------- attr 17.890M (± 3.0%) i/s - 89.446M in 5.004440s def 15.291M (± 2.5%) i/s - 76.552M in 5.009543s Comparison: attr: 17890288.4 i/s def : 15291283.1 i/s - 1.17x slower

dillonwelch · 2018-03-30T19:28:11Z

Reverted the symbol proc change. Out of the remaining Rubocop errors:
activerecord/test/cases/relation_test.rb This is a false flag. When I make the change, the specs fail. It looks like merge! is an ActiveRecord method that doesn't have the same behavior.

actionpack/test/dispatch/header_test.rb These two tests explicitly call out merge! and that the test is validating side effects. Wasn't sure if we wanted to edit the test or maybe get rid of it entirely?

activesupport/lib/active_support/testing/parallelization.rb and railties/test/app_loader_test.rb These are class level methods, not instance level. I wasn't sure if these were a false flag or not.

rafaelfranca · 2018-03-30T19:50:23Z

r? @matthewd

bdewater · 2018-04-01T00:00:51Z

@schneems regarding Style/SymbolProc there's useful discussion in #16833 where a similar change was made in the past by @sferik. As remarked there the difference becomes noticeable on larger arrays. Your benchmark example with 12 instead of 3 array elements makes regular proc 1.20x slower. IMO it's good to just turn in one for consistency's sake.

FWIW the benchmark results in https://github.com/JuanitoFatas/fast-ruby#proc--block still hold true on Ruby 2.4.3 and 2.5.0 on my laptop as well.

kaspth · 2018-04-01T13:19:03Z

I'm in favor of turning the SymbolProc on, so we have that consistent style throughout exactly because of #16833. But we can keep it for another PR, so this is easier to review.

dillonwelch · 2018-04-13T23:55:28Z

@matthewd anything I can do to help get this over the finish line?

przemosk · 2018-04-19T08:57:22Z

actionview/test/template/form_helper_test.rb

@@ -168,7 +168,8 @@ def url_for(object)
    @url_for_options = object

    if object.is_a?(Hash) && object[:use_route].blank? && object[:controller].blank?
-      object.merge!(controller: "main", action: "index")
+      object[:controller] = "main"


@oniofchaos please use " only for interpolation

That's the style I use in my own projects but it's not the style for the Rails codebase. You can see the Rubocop failures here: https://codeclimate.com/github/rails/rails/pull/32381

przemosk · 2018-04-19T08:57:31Z

actionview/test/template/form_helper_test.rb

@@ -168,7 +168,8 @@ def url_for(object)
    @url_for_options = object

    if object.is_a?(Hash) && object[:use_route].blank? && object[:controller].blank?
-      object.merge!(controller: "main", action: "index")
+      object[:controller] = "main"
+      object[:action] = "index"


@oniofchaos same here

@oniofchaos
FYI if you have rubocop gem, you should be warned when writing this, based on following rubocop configuration https://github.com/rails/rails/blob/master/.rubocop.yml#L128

The enforced style is double_quotes (docs) so there should be no warning.

schneems · 2018-07-22T20:53:35Z

Seems fine. Sorry for taking so long. Can you rebase, fix conflicts and I will merge in. Let me know if you don’t know how to do that and need some help.

dillonwelch · 2018-07-23T18:17:59Z

@schneems fixed!

schneems · 2018-07-23T20:15:12Z

Thanks! Looks like codeclimate isn’t happy about a few things. Can you look into those?

dillonwelch · 2018-07-23T20:49:42Z

I fixed the 3 attr_reader errors. For the three merge! errors, based on the test description it looks like they are explicitly testing the effects of merge! so I'm not sure that it makes sense to edit them to not use merge!?

schneems · 2018-07-23T21:27:35Z

Can we skip those rules inline? Alternatively, we could get around it with send but that's pretty hacky. Disabling the check for these 3 cases seems appropriate.

schneems · 2018-07-23T22:09:27Z

Restarting to some failures in railties. Not sure if they're intermittent or not

Error:
AppLoaderTest#test_is_in_a_Rails_application_if_parent_directory_has_script/rails_containing_APP_PATH_and_chdirs_to_the_root_directory:
NoMethodError: undefined method `exec_arguments' for #<Class:0x0000000002de0d58>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:73:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:65
.....E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_parent_directory_has_bin/rails_containing_APP_PATH_and_chdirs_to_the_root_directory:
NoMethodError: undefined method `exec_arguments' for #<Class:0x0000000002652cf8>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:73:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:65
..E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_bin/rails_exists_and_contains_APP_PATH:
NoMethodError: undefined method `exec_arguments' for #<Class:0x0000000004284b10>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:56:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:51
E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_parent_directory_has_bin/rails_containing_ENGINE_PATH_and_chdirs_to_the_root_directory:
NoMethodError: undefined method `exec_arguments' for #<Class:0x0000000004263c30>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:73:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:65
E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_bin/rails_exists_and_contains_ENGINE_PATH:
NoMethodError: undefined method `exec_arguments' for #<Class:0x0000000004245140>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:56:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:51
E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_script/rails_exists_and_contains_APP_PATH:
NoMethodError: undefined method `exec_arguments' for #<Class:0x000000000421f260>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:56:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:51
E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_script/rails_exists_and_contains_ENGINE_PATH:
NoMethodError: undefined method `exec_arguments' for #<Class:0x00000000041f9510>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:56:in `block (3 levels) in <class:AppLoaderTest>'
bin/rails test test/app_loader_test.rb:51
E
Error:
AppLoaderTest#test_is_in_a_Rails_application_if_parent_directory_has_script/rails_containing_ENGINE_PATH_and_chdirs_to_the_root_directory:
NoMethodError: undefined method `exec_arguments' for #<Class:0x00000000041ce568>
    test/app_loader_test.rb:26:in `expects_exec'
    test/app_loader_test.rb:73:in `block (3 levels) in <class:AppLoaderTest>'
0m
bin/rails test test/app_loader_test.rb:65
.
Finished in 0.049501s, 323.2226 runs/s, 161.6113 assertions/s.
16 runs, 8 assertions, 0 failures, 8 errors, 0 skips
Failed in:
  test/app_loader_test.rb
rake aborted!
Failure in isolated test runner
/home/travis/build/rails/rails/railties/Rakefile:70:in `block (2 levels) in <top (required)>'
/home/travis/.rvm/gems/ruby-2.4.4/gems/rake-12.3.1/exe/rake:27:in `<top (required)>'
/home/travis/.rvm/gems/ruby-2.4.4/bin/ruby_executable_hooks:24:in `eval'
/home/travis/.rvm/gems/ruby-2.4.4/bin/ruby_executable_hooks:24:in `<main>'
Tasks: TOP => test => test:isolated
(See full trace by running task with --trace)
Rails build FAILED
Failed components: railties
The command "ci/travis.rb" exited with 1.
cache.2
store build cache
0.01s
1.76snothing changed, not updating cache
Done. Your build exited with 1.

schneems · 2018-07-23T22:36:57Z

Still failing.

Use attr_reader/attr_writer instead of methods method is 12% slower Use flat_map over map.flatten(1) flatten is 66% slower Use hash[]= instead of hash.merge! with single arguments merge! is 166% slower See rails#32337 for more conversation

dillonwelch · 2018-07-23T22:37:55Z

Just pushed up a hopeful fix

dillonwelch · 2018-07-23T23:09:57Z

I'm not sure how to trigger a rebuild, but it looks like the failure this time was an intermittent issue and not from my code.

kaspth · 2018-07-25T18:15:34Z

actionpack/test/dispatch/header_test.rb

@@ -105,16 +105,20 @@ def make_headers(hash)
  end

  test "#merge! headers with mutation" do
+    # rubocop:disable Performance/RedundantMerge


One of the core teams' design goals when adding Rubocop was that we never wanted to add these disable comments. So we've intentionally turned on a light set of cops to give us some enforcement, but not a straitjacket where we'd have to strip the buttons off again.

Let's find another to write this or we just don't enable that cop.

Based on my reading here, it appears this is testing external use of merge! which we don't have control over. I could rewrite it to use the performance improvement version but it seems to go against the spirit of the test.

The only other way would be “send”. I think that’s worse than the comment and it’s only in a handful of places.

We also can turn off Performance/RedundantMerge for these files via Exclude in .rubocop.yml file.
https://github.com/rubocop-hq/rubocop/blob/master/manual/configuration.md#includingexcluding-files

schneems · 2018-07-25T21:00:26Z

Going to merge for now. If there is a preferred way for dealing with those rubocop comment exceptions we can update in the future.

Thanks for all your work here!

Follow up of rails/rails#32381. In addition, this PR applies the following auto-correct. ```console % rubocop -a Inspecting 64 files ..........C..................................................... Offenses: spec/active_record/connection_adapters/oracle_enhanced/schema_dumper_spec.rb:22:5: C: [Corrected] Performance/RedundantMerge: Use options[:force] = true instead of options.merge! force: true. options.merge! force: true ^^^^^^^^^^^^^^^^^^^^^^^^^^ 64 files inspected, 1 offense detected, 1 offense corrected ```

Follow up of rails/rails#32381.

PR#32381 added Rubocop's comments to some tests files in order to exclude `Performance/RedundantMerge`. Turn off `Performance` cops for tests files via `Exclude` in `.rubocop.yml`. Context rails#32381 (comment)

rails-bot assigned schneems Mar 30, 2018

dillonwelch force-pushed the update-codeclimate-configs branch 4 times, most recently from e847575 to 38c9355 Compare March 30, 2018 05:47

dillonwelch force-pushed the update-codeclimate-configs branch 3 times, most recently from 155b178 to 0f11c31 Compare March 30, 2018 06:45

qinmingyuan reviewed Mar 30, 2018

View reviewed changes

schneems suggested changes Mar 30, 2018

View reviewed changes

dillonwelch force-pushed the update-codeclimate-configs branch from 0f11c31 to 0f96701 Compare March 30, 2018 19:15

rails-bot assigned matthewd and unassigned schneems Mar 30, 2018

przemosk suggested changes Apr 19, 2018

View reviewed changes

dillonwelch force-pushed the update-codeclimate-configs branch from 654a422 to 9793473 Compare July 23, 2018 17:44

dillonwelch force-pushed the update-codeclimate-configs branch from 9793473 to 3f39719 Compare July 23, 2018 20:48

dillonwelch force-pushed the update-codeclimate-configs branch from 3f39719 to 6322693 Compare July 23, 2018 21:38

Turn on performance based cops

d108288

Use attr_reader/attr_writer instead of methods method is 12% slower Use flat_map over map.flatten(1) flatten is 66% slower Use hash[]= instead of hash.merge! with single arguments merge! is 166% slower See rails#32337 for more conversation

dillonwelch force-pushed the update-codeclimate-configs branch from 6322693 to d108288 Compare July 23, 2018 22:37

kaspth reviewed Jul 25, 2018

View reviewed changes

schneems merged commit 91fd679 into rails:master Jul 25, 2018

dillonwelch deleted the update-codeclimate-configs branch July 25, 2018 21:08

koic added a commit to koic/rubocop-rails_config that referenced this pull request Jul 26, 2018

Turn on performance based cops

8f79cc8

Follow up of rails/rails#32381.

koic mentioned this pull request Jul 26, 2018

Turn on performance based cops rsim/oracle-enhanced#1734

Merged

koic added a commit to koic/rubocop-rails_config that referenced this pull request Jul 26, 2018

Turn on performance based cops

522b4b0

Follow up of rails/rails#32381.

koic mentioned this pull request Jul 26, 2018

Turn on performance based cops toshimaru/rubocop-rails_config#9

Merged

bogdanvlviv mentioned this pull request Jul 26, 2018

Remove Rubocop's comments from Rails code base #33441

Merged

dillonwelch mentioned this pull request Sep 28, 2018

ActiveCable support for rack/rack PR #1272 #33295

Closed

bogdanvlviv mentioned this pull request Dec 21, 2018

Fix app boot file for Ruby 2.4 so it tells you to install Ruby 2.5 #34774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on performance based cops #32381

Turn on performance based cops #32381

dillonwelch commented Mar 30, 2018

rails-bot commented Mar 30, 2018

dillonwelch commented Mar 30, 2018

qinmingyuan Mar 30, 2018 •

edited

schneems Mar 30, 2018

schneems Mar 30, 2018

schneems left a comment

schneems Mar 30, 2018

schneems Mar 30, 2018

schneems Mar 30, 2018

dillonwelch commented Mar 30, 2018

rafaelfranca commented Mar 30, 2018

bdewater commented Apr 1, 2018 •

edited

kaspth commented Apr 1, 2018

dillonwelch commented Apr 13, 2018

przemosk Apr 19, 2018

dillonwelch Apr 19, 2018

przemosk Apr 19, 2018

saiqulhaq Apr 19, 2018 •

edited

bdewater Apr 19, 2018

schneems commented Jul 22, 2018

dillonwelch commented Jul 23, 2018

schneems commented Jul 23, 2018

dillonwelch commented Jul 23, 2018

schneems commented Jul 23, 2018

schneems commented Jul 23, 2018 •

edited

schneems commented Jul 23, 2018

dillonwelch commented Jul 23, 2018

dillonwelch commented Jul 23, 2018

kaspth Jul 25, 2018

dillonwelch Jul 25, 2018

schneems Jul 25, 2018

bogdanvlviv Jul 25, 2018

schneems commented Jul 25, 2018

Turn on performance based cops #32381

Turn on performance based cops #32381

Conversation

dillonwelch commented Mar 30, 2018

Summary

rails-bot commented Mar 30, 2018

dillonwelch commented Mar 30, 2018

qinmingyuan Mar 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneems left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dillonwelch commented Mar 30, 2018

rafaelfranca commented Mar 30, 2018

bdewater commented Apr 1, 2018 • edited

kaspth commented Apr 1, 2018

dillonwelch commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saiqulhaq Apr 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneems commented Jul 22, 2018

dillonwelch commented Jul 23, 2018

schneems commented Jul 23, 2018

dillonwelch commented Jul 23, 2018

schneems commented Jul 23, 2018

schneems commented Jul 23, 2018 • edited

schneems commented Jul 23, 2018

dillonwelch commented Jul 23, 2018

dillonwelch commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schneems commented Jul 25, 2018

qinmingyuan Mar 30, 2018 •

edited

bdewater commented Apr 1, 2018 •

edited

saiqulhaq Apr 19, 2018 •

edited

schneems commented Jul 23, 2018 •

edited