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

Test for emptiness is very slow #5947

Open
2 tasks done
KeithBauerANZ opened this issue Jan 10, 2025 · 7 comments
Open
2 tasks done

Test for emptiness is very slow #5947

KeithBauerANZ opened this issue Jan 10, 2025 · 7 comments
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions.

Comments

@KeithBauerANZ
Copy link

New Issue Checklist

Bug Description

I work on a project with a large number (~12000) of swift files. SwiftLint's cache does a good job of avoiding linting them when they haven't changed, which means that vast majority of the time we spend waiting for swiftlint is in its "setup costs". One of those costs is in determining whether to avoid linting the file because it is empty. This is done by this function at the bottom of Linter.swift:

private extension SwiftLintFile {
    var isEmpty: Bool {
        contents.isEmpty || contents == "\n"
    }
}

Because this accesses the contents property, this forces reading all 12000 swift files into memory, just to know if they're empty or not. This costs several seconds of execution time. That doesn't matter if they're then going to be linted anyway, but it does matter when they're unchanged from the last run and will skip linting.

I intended to contribute a fix for this, but it's not at all obvious to me what a good approach would be — this sits right at the intersection of SwiftLint and SourceKitten. Perhaps we should gather file sizes during the directory traversal, store them somewhere, and only read the file if the size == 1?

Environment

SwiftLint version (run swiftlint version to be sure)
0.57.1

Xcode version (run xcodebuild -version to be sure)
Xcode 16.2
Build version 16C5032a

Installation method used (Homebrew, CocoaPods, building from source, etc)
Downloaded from release tag

Configuration file:
n/a

@SimplyDanny
Copy link
Collaborator

From what I see, the cache is checked before the content is tested for emptiness. So how did you come up with the assumption that the isEmpty always kicks in? Can you elaborate on that?

@SimplyDanny SimplyDanny added the discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. label Jan 10, 2025
@KeithBauerANZ
Copy link
Author

I used Instruments to profile swiftlint, when running on my own project. The call from Rule.lint is skipped if the results are cached, but the call from Linter.init doesn't seem to be.

@KeithBauerANZ
Copy link
Author

Image

Here's a screenshot showing what I'm seeing. The highlighted region & inverted stack trace shows that this ~2 seconds is dominated by calls to open from isEmpty.

(The other regions of low CPU usage are also interesting, but I had more idea how to fix them)

@KeithBauerANZ
Copy link
Author

(I also confirmed that by replacing isEmpty's body by false, this section of the profile more or less vanishes)

@SimplyDanny
Copy link
Collaborator

After a brief glimpse on the code, it looks like the isEmpty check in Linter.init can just be removed. Without the filter, all rules would be collected. But before they do anything, there is the other check for isEmpty. It even seems cleaner to let the rule decide whether it wants to do things with empty files or skip them. Even better, this already happens in a parallel context.

Looks like we could give this a try.

@OneSadCookie
Copy link

OneSadCookie commented Jan 18, 2025

diff --git a/Source/SwiftLintFramework/Models/Linter.swift b/Source/SwiftLintFramework/Models/Linter.swift
index 68f111fdc..5c85aad4c 100644
--- a/Source/SwiftLintFramework/Models/Linter.swift
+++ b/Source/SwiftLintFramework/Models/Linter.swift
@@ -226,12 +226,7 @@ public struct Linter {
         self.configuration = configuration
         self.compilerArguments = compilerArguments
 
-        let fileIsEmpty = file.isEmpty
         let rules = configuration.rules.filter { rule in
-            if fileIsEmpty, !rule.shouldLintEmptyFiles {
-                return false
-            }
-
             if compilerArguments.isEmpty {
                 return !(rule is any AnalyzerRule)
             }

Unfortunately, that results in changed user-facing behavior:

Test Case '-[IntegrationTests.IntegrationTests testSwiftLintAutoCorrects]' started.
/Users/....../SwiftLint/Source/SourceKittenFrameworkWrapper/Empty.swift:1: error: -[IntegrationTests.IntegrationTests testSwiftLintAutoCorrects] : failed - Files should have a single trailing newline
Test Case '-[IntegrationTests.IntegrationTests testSwiftLintAutoCorrects]' failed (38.332 seconds).

@SimplyDanny
Copy link
Collaborator

Unfortunately, that results in changed user-facing behavior:

Test Case '-[IntegrationTests.IntegrationTests testSwiftLintAutoCorrects]' started.
/Users/....../SwiftLint/Source/SourceKittenFrameworkWrapper/Empty.swift:1: error: -[IntegrationTests.IntegrationTests testSwiftLintAutoCorrects] : failed - Files should have a single trailing newline
Test Case '-[IntegrationTests.IntegrationTests testSwiftLintAutoCorrects]' failed (38.332 seconds).

This is a bug. While linting checks for emptiness in combination with shouldLintEmptyFiles, auto-correction doesn't care. This is not okay; auto-correction should never fix more than what would be reported by linting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions.
Projects
None yet
Development

No branches or pull requests

3 participants