-
Notifications
You must be signed in to change notification settings - Fork 503
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
0.41.0 Performance decrease #1135
Comments
@greggiacovelli would you be able to test with the latest SNAPSHOT and see if this is resolved? We merged #1144 which may help. |
@shashachu Nice! Simple anecdotal test of changing a single file in the repo with a bunch of spaces and imports reordered. And then invoking ktlint on just the file, resulted in 1 sec time until output
Results in:
vs with no file arguments, evaluates the entire codebase (which is expected)
Results in:
Seems fixed to me |
@greggiacovelli great. thanks so much for confirming, and thank you to @rciovati for the fix! |
Will be released in 0.42.0 (ETA ~1-2 weeks.) |
Fixes a nasty performance bug: pinterest/ktlint#1135
@shashachu It's not fixed to me, the issue should be reopened. Version 0.40.0
Version 0.41.0
Version 0.42.0
Version 0.43.2
Tested with the same two files. It's painful if you have a pre-commit hook. Environment:
|
It's definitely slower but I think the 30 seconds from the past is fixed. I have found it required to either specify all files that need to be formatted/checked via pre-commit or when using it on a code base to prune any stale directories as this adds to the issue. Cpu is not very high so yeah it seems like it's traversing files in your case |
🤦 You are right! This is the pre-commit hook installed by ktlint: #!/bin/sh
# https://github.com/shyiko/ktlint pre-commit hook
git diff --name-only --cached --relative | grep '\.kt[s"]\?$' | xargs ktlint --relative .
if [ $? -ne 0 ]; then exit 1; fi Am I wrong? |
Yeah I have been using pre-commit to manage my pre commit hooks. It searches the git index and can pass the exec path only the files in question. https://github.com/macisamuele/language-formatters-pre-commit-hooks has a collection of hooks for this system that works really well |
I was thinking about this. Even if my pre-commit hook was traversing all files, there is a tremendous performance decrease from 0.40.0 to 0.41.0, 0.42.0, 0.43.2. I haven't changed the pre-commit hook while performing my tests, which means something has clearly changed in ktlint affecting performances. I ran my tests under the same conditions, just switching ktlint release. Sure, it doesn't make any sense to traverse all files if you just want to check changed files, but 0.40.0 was way faster than 0.41/0.42/0.43 in doing so. |
Ah sorry my However that might not function the same after the changes in 0.41.0 as if I recall the way paths were resolved had been re-implemented. The resulting hook may have been updated too to account for the new behaviors. However it's possible it may not have been updated too So after re-installing the hook, are the same symptoms present? |
I don't think that the current directory is an argument for
Hooks were never updated after 0.40.0. The one I pasted here is the same from 0.40 to 0.43. |
I think then we found the issue. If after making the change (removing the |
Yes, it works. Performance is comparable to the one observed in 0.40.0. |
@francescocervone You may need to create a new issue to address what you are seeing as it is not 100% related to this issue. |
Expected Behavior
When updating between versions, I expected comparable performance.
Observed Behavior
Upgrading from 0.40.0 to 0.41.0 decreases performance considerably. I noticed this from using a precommit hook and the time to commit even a file without any formatting issues, went up considerably. When using Debug flags on
0.41.0
the print outs stop at:[DEBUG] Initializing "plain" reporter with {verbose=true, color=false, color_name=DARK_GRAY}
where it takes the majority of the observed lag (~30+ seconds)
Steps to Reproduce
Now with the same file in the same state:
lsof
of the process while in this state shows it trying to traverse the entire directory structure, whereas before this would short circuit only to the files on the command line. I understand the glob logic changed, however something seems up, even if it eventually gets to the right file, it seems to take a rather long time to find it.Your Environment
The text was updated successfully, but these errors were encountered: