Small `Lint` cops perf tweaks #8358

fatkodima · 2020-07-18T01:43:21Z

Ran on larger discourse codebase (3.5k files).

$ bin/rubocop-profile --force-default-config --cache false --only Lint ../discourse

Before

 1   965  (    7.3%)  RuboCop::Cop::Lint::DeprecatedClassMethods#on_send
 2   952  (    7.2%)  RuboCop::Cop::Lint::Debugger#on_send
 3   806  (    6.1%)  RuboCop::Cop::Lint::AmbiguousBlockAssociation#on_send
     642  (    4.8%)  RuboCop::Cop::Lint::ParenthesesAsGroupedExpression#on_send
     623  (    4.7%)  RuboCop::Cop::Lint::SafeNavigationChain#on_send
 4   605  (    4.6%)  RuboCop::Cop::Lint::FormatParameterMismatch#on_send
 5   533  (    4.4%)  RuboCop::Cop::Lint::DuplicateMethods#on_send
 6   500  (    3.8%)  RuboCop::Cop::Lint::InterpolationCheck#on_str
 7   493  (    3.7%)  RuboCop::Cop::Lint::IneffectiveAccessModifier#on_class
 8   473  (    3.6%)  RuboCop::Cop::Lint::BigDecimalNew#on_send
     467  (    3.5%)  RuboCop::Cop::Lint::RequireParentheses#on_send
 9   464  (    3.5%)  RuboCop::Cop::Lint::UriRegexp#on_send
     403  (    3.0%)  RuboCop::Cop::Lint::MultipleComparison#on_send
     376  (    2.8%)  RuboCop::Cop::Lint::Void#on_begin
     332  (    2.5%)  RuboCop::Cop::Lint::UriEscapeUnescape#on_send
     313  (    2.4%)  RuboCop::Cop::Lint::UnreachableCode#on_begin
     312  (    2.4%)  RuboCop::Cop::Lint::DuplicateMethods#on_def
10   312  (    2.4%)  RuboCop::Cop::Lint::InheritException#on_send
     299  (    2.3%)  RuboCop::Cop::Lint::UselessComparison#on_send
     296  (    2.2%)  RuboCop::Cop::Lint::DuplicateHashKey#on_hash
     259  (    2.0%)  RuboCop::Cop::Lint::EachWithObjectArgument#on_send
11   252  (    1.9%)  RuboCop::Cop::Lint::RandOne#on_send
     228  (    1.7%)  RuboCop::Cop::Lint::LiteralAsCondition#on_send

After

     635  (    7.3%)  RuboCop::Cop::Lint::SafeNavigationChain#on_send
     597  (    6.9%)  RuboCop::Cop::Lint::ParenthesesAsGroupedExpression#on_send
     451  (    5.2%)  RuboCop::Cop::Lint::RequireParentheses#on_send
3    404  (    4.7%)  RuboCop::Cop::Lint::AmbiguousBlockAssociation#on_send
     346  (    4.0%)  RuboCop::Cop::Lint::Void#on_begin
5    320  (    3.7%)  RuboCop::Cop::Lint::DuplicateMethods#on_def
     312  (    3.6%)  RuboCop::Cop::Lint::UriEscapeUnescape#on_send
     303  (    3.5%)  RuboCop::Cop::Lint::UselessComparison#on_send
     291  (    3.4%)  RuboCop::Cop::Lint::UnreachableCode#on_begin
     280  (    3.2%)  RuboCop::Cop::Lint::MultipleComparison#on_send
     275  (    3.2%)  RuboCop::Cop::Lint::EachWithObjectArgument#on_send
     263  (    3.0%)  RuboCop::Cop::Lint::DuplicateHashKey#on_hash
     250  (    2.9%)  RuboCop::Cop::Lint::SendWithMixinArgument#on_send
7    240  (    2.8%)  RuboCop::Cop::Lint::IneffectiveAccessModifier#on_class
6    239  (    2.5%)  RuboCop::Cop::Lint::InterpolationCheck#on_str
     188  (    2.2%)  RuboCop::Cop::Lint::ImplicitStringConcatenation#on_dstr
     187  (    2.2%)  RuboCop::Cop::Lint::RedundantRequireStatement#on_send
8    184  (    2.1%)  RuboCop::Cop::Lint::BigDecimalNew#on_send
2    182  (    2.1%)  RuboCop::Cop::Lint::Debugger#on_send
     176  (    2.0%)  RuboCop::Cop::Lint::DuplicateMethods#on_send
     168  (    1.9%)  RuboCop::Cop::Lint::UselessAccessModifier#on_class
     166  (    1.9%)  RuboCop::Cop::Lint::LiteralAsCondition#on_send
 9   159  (    1.8%)  RuboCop::Cop::Lint::UriRegexp#on_send
 1   143  (    1.7%)  RuboCop::Cop::Lint::DeprecatedClassMethods#on_send
 4   140  (    1.6%)  RuboCop::Cop::Lint::FormatParameterMismatch#on_send
     113  (    1.3%)  RuboCop::Cop::Lint::AssignmentInCondition#on_if
      91  (    1.1%)  RuboCop::Cop::Lint::UselessAccessModifier#on_block

marcandre · 2020-07-18T06:32:28Z

lib/rubocop/cop/lint/debugger.rb

+          debugger byebug remote_byebug pry remote_pry pry_remote console rescue
+          save_and_open_page save_and_open_screenshot save_screenshot irb
+        ].to_set.freeze
+
        def_node_matcher :kernel?, <<~PATTERN


I want to make a release of rubocop-ast soon so we can use #global_const? :-)

Should we also add #kernel_call? == {#global_const?(:Kernel) nil?} or similar to rubocop-ast?

bbatsov · 2020-07-18T06:35:14Z

Nicely done! 🙇‍♂️

marcandre · 2020-07-18T06:40:35Z

lib/rubocop/cop/lint/ambiguous_block_association.rb

@@ -30,10 +30,11 @@ class AmbiguousBlockAssociation < Cop
              'call.'

        def on_send(node)
-          return if !node.arguments? || node.parenthesized? ||
-                    node.last_argument.lambda? || allowed_method?(node)
+          return unless node.arguments?


I was surprised to see that this line was still taking non-negligible time, so I investigated a bit and this led to a bunch of fixes and a bunch of optimizations in rubocop-ast 🎉

marcandre · 2020-07-18T06:41:30Z

lib/rubocop/cop/lint/debugger.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true

+require 'set'


No need to require 'set' here, it's required by parser and (just to be explicit) by rubocop-ast too.

marcandre · 2020-07-18T06:41:52Z

lib/rubocop/cop/lint/duplicate_methods.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true

+require 'set'


marcandre · 2020-07-18T06:42:41Z

lib/rubocop/cop/lint/duplicate_methods.rb

        def on_send(node)
+          return unless METHOD_DEF_METHODS.include?(node.method_name)
+
          if (name = alias_method?(node))
            return unless name


This line is redundant. Apparently we don't have a cop for this 😅

marcandre · 2020-07-18T06:54:01Z

lib/rubocop/cop/lint/interpolation_check.rb

@@ -21,11 +21,10 @@ class InterpolationCheck < Cop
              'Use double quoted strings if you need interpolation.'

        def on_str(node)
-          return if heredoc?(node)
-
          parent = node.parent
          return if parent && (parent.dstr_type? || parent.regexp_type?)
          return unless /(?<!\\)#\{.*\}/.match?(node.source.scrub)


AFAIK, parser only handles valid utf-8 sequences, so scrub should not be needed at all here, or am I missing something?

marcandre

Good work 💪

A few minor comments for you

marcandre · 2020-07-18T07:00:53Z

Oh, merged already 😅

bbatsov · 2020-07-18T07:03:39Z

raise ConcurrentReviewException 😉

Small Lint cops perf tweaks

8d9a799

marcandre reviewed Jul 18, 2020

View reviewed changes

bbatsov merged commit cb57295 into rubocop:master Jul 18, 2020

marcandre reviewed Jul 18, 2020

View reviewed changes

lib/rubocop/cop/lint/duplicate_methods.rb

@@ -1,5 +1,7 @@

# frozen_string_literal: true

require 'set'

Copy link

Contributor

marcandre Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need

marcandre reviewed Jul 18, 2020

View reviewed changes

marcandre requested changes Jul 18, 2020

View reviewed changes

marcandre mentioned this pull request Jul 20, 2020

Tweaks #8371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small `Lint` cops perf tweaks #8358

Small `Lint` cops perf tweaks #8358

fatkodima commented Jul 18, 2020

marcandre Jul 18, 2020 •

edited

bbatsov commented Jul 18, 2020

marcandre Jul 18, 2020

marcandre Jul 18, 2020

marcandre Jul 18, 2020

marcandre Jul 18, 2020

marcandre Jul 18, 2020

marcandre left a comment

marcandre commented Jul 18, 2020

bbatsov commented Jul 18, 2020

Small Lint cops perf tweaks #8358

Small Lint cops perf tweaks #8358

Conversation

fatkodima commented Jul 18, 2020

Before

After

marcandre Jul 18, 2020 • edited

Choose a reason for hiding this comment

bbatsov commented Jul 18, 2020

marcandre Jul 18, 2020

Choose a reason for hiding this comment

marcandre Jul 18, 2020

Choose a reason for hiding this comment

marcandre Jul 18, 2020

Choose a reason for hiding this comment

marcandre Jul 18, 2020

Choose a reason for hiding this comment

marcandre Jul 18, 2020

Choose a reason for hiding this comment

marcandre left a comment

Choose a reason for hiding this comment

marcandre commented Jul 18, 2020

bbatsov commented Jul 18, 2020

Small `Lint` cops perf tweaks #8358

Small `Lint` cops perf tweaks #8358

marcandre Jul 18, 2020 •

edited