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

Performance issues with 1.31.0 (Layout/FirstHashElementIndentation and Layout/FirstArrayElementIndentation) #10764

Closed
seandilda opened this issue Jun 28, 2022 · 5 comments · Fixed by #10777

Comments

@seandilda
Copy link

After updating to 1.31.0, my editor started complaining about rubocop timeouts. Some testing has narrowed the issue to the recent changes in Layout/FirstHashElementIndentation and Layout/FirstArrayElementIndentation.

Here are some timings from running time rubocop --cache false testfile.rb on a file I'm currently working on.

rubocop version Layout/FirstHashElementIndentation enabled Layout/FirstArrayElementIndentation enabled time
1.30.1 true true 3.5s
1.31.0 true true 9.3s
1.31.0 true false 5.3s
1.31.0 false true 7.8s
1.31.0 false false 3.4s

Expected behavior

RuboCop 1.31.0 should run at the same speed as 1.30.1

Actual behavior

Version 1.31.0 is several times slower when Layout/FirstHashElementIndentation and/or Layout/FirstArrayElementIndentation is enabled

Steps to reproduce the problem

Run rubocop --cache false on a file using a large number of hashes and arrays.

RuboCop version

1.31.0 (using Parser 3.1.2.0, rubocop-ast 1.18.0, running on ruby 2.7.6 x86_64-linux)
  - rubocop-rails 2.15.1
@dvandersluis
Copy link
Member

I'm curious if you get the same results now that 1.31.1 was released? Did #10750 help?

@seandilda
Copy link
Author

Thanks for the heads up! I just checked with 1.31.1 and I'm still seeing 9 second runs on that file. :(

@dvandersluis
Copy link
Member

Are you able to share the file or an anonymized version of it?

@seandilda
Copy link
Author

I've created a test file to reproduce this issue at https://gist.github.com/seandilda/a5872778c19fd82624809d856f0b7435

With 1.31.1, and both cops enabled, it takes 4.5s
With 1.31.1 and both cops disabled, it takes 2.1s
With 1.30.1 and both cops enabled, it takes 2.1s

@j-miyake
Copy link
Contributor

j-miyake commented Jun 29, 2022

I guess a part of the logic for searching an offensive node is expensive. For a small file, that may not be a big deal, but for a large file.

def node_beginning_with(left_brace)
processed_source.ast.each_descendant do |node|
if node.loc.is_a?(Parser::Source::Map::Collection) && (node.loc.begin == left_brace)
break node
end
end
end

j-miyake added a commit to j-miyake/rubocop that referenced this issue Jun 30, 2022
…Indentation and Layout/FirstArrayElementIndentation

Fixes rubocop#10764.

In rubocop v1.31.0, `MultilineElementIndentation` that is used by `Layout/FirstHashElementIndentation` and `Layout/FirstArrayElementIndentation` walks all nodes through in the code to find a hash pair node where its value part begins with the given left-brace; That computation is expensive, so I made a change so that the hash pair is passed by each class that includes `MultilineElementIndentation`.
bbatsov pushed a commit that referenced this issue Jul 7, 2022
…tion and Layout/FirstArrayElementIndentation

Fixes #10764.

In rubocop v1.31.0, `MultilineElementIndentation` that is used by `Layout/FirstHashElementIndentation` and `Layout/FirstArrayElementIndentation` walks all nodes through in the code to find a hash pair node where its value part begins with the given left-brace; That computation is expensive, so I made a change so that the hash pair is passed by each class that includes `MultilineElementIndentation`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants