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

Add new Performance/RedundantStringChars cop #141

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

fatkodima
Copy link
Contributor

@fatkodima fatkodima commented Jun 21, 2020

Closes #139

No need to call #chars and allocate an extra array when method can be directly called on string.

# bad
str.chars[0..2]
str.chars.slice(0..2)

# good
str[0..2].chars

# bad
str.chars.first
str.chars.first(2)
str.chars.last
str.chars.last(2)

# good
str[0]
str[0...2].chars
str[-1]
str[-2..-1].chars

# bad
str.chars.take(2)
str.chars.drop(2)
str.chars.length
str.chars.size
str.chars.empty?

# good
str[0...2].chars
str[2..-1].chars
str.length
str.size
str.empty?

Benchmark

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  gem 'benchmark-ips'
  gem 'benchmark-memory'
end

STR = 'a' * 20
RANGE = 10..15


puts('********* IPS *********')

Benchmark.ips do |x|
  x.report("String#chars[...]")   { STR.chars[RANGE] }
  x.report("String#[...]")        { STR[RANGE] }
  x.compare!
end

puts "********* MEMORY *********"

Benchmark.memory do |x|
  x.report("String#chars[...]")   { STR.chars[RANGE] }
  x.report("String#[...]")        { STR[RANGE] }
  x.compare!
end

Results

********* IPS *********
Warming up --------------------------------------
   String#chars[...]    18.063k i/100ms
        String#[...]   214.729k i/100ms
Calculating -------------------------------------
   String#chars[...]    181.304k (± 3.5%) i/s -    921.213k in   5.087585s
        String#[...]      2.141M (± 2.2%) i/s -     10.736M in   5.016910s

Comparison:
        String#[...]:  2141163.0 i/s
   String#chars[...]:   181304.1 i/s - 11.81x  (± 0.00) slower

********* MEMORY *********
Calculating -------------------------------------
   String#chars[...]   920.000  memsize (     0.000  retained)
                        23.000  objects (     0.000  retained)
                         2.000  strings (     0.000  retained)
        String#[...]    40.000  memsize (     0.000  retained)
                         1.000  objects (     0.000  retained)
                         1.000  strings (     0.000  retained)

Comparison:
        String#[...]:         40 allocated
   String#chars[...]:        920 allocated - 23.00x more

@fatkodima
Copy link
Contributor Author

Updated.

@fatkodima fatkodima changed the title Add new Performance/StringChars cop Add new Performance/RedundantStringChars cop Jun 23, 2020
@fatkodima
Copy link
Contributor Author

I thought about it a little more. This cop was originally created to primarily catch cases like str.chars[0..2] (and equivalent str.chars.slice(0..2)). This was originally implemented (incorrectly) and then dropped. I have added support to replace them with faster alternative, str[0..2].chars.

And also updated implementation to catch #take(n), #drop(n); #first and #last with argument.

config/default.yml Outdated Show resolved Hide resolved
@fatkodima
Copy link
Contributor Author

Updated.

@koic koic merged commit 2757251 into rubocop:master Jun 29, 2020
@koic
Copy link
Member

koic commented Jun 29, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need cop against chars.slice and al.
2 participants