Make getting style violations thread-safe and parallelize linting #1103

jpsim · 2016-12-31T07:39:12Z

Addresses #1077. Thread sanitizer seems happy with this when running unit tests and linting a few projects I could find, including the Swift standard library.

Generally speeds up linting by ~30% to ~140% on my 2013 MacBook Pro, though the gains should be higher on more recent CPUs.

apple/swift: 218s to 166s (31% faster)
realm/realm-cocoa: 9.6s to 5.6s (71% faster)
Carthage/Carthage: 96s to 53s (80% faster)
jpsim/SourceKitten: 4.2s to 2.1s (100% faster)
realm/SwiftLint: 6.8s to 3.3s (136% faster)

I had initially added a --parallel flag to the lint command, but have since removed that option since I haven't run into any issues with it so far, and couldn't come up with a compelling reason to hide it behind an opt-in flag.

and make some of its members private instead of fileprivate

SwiftLintBot · 2016-12-31T07:45:32Z

	1 Warning
⚠️	This PR may need tests.

Generated by 🚫 danger

krzysztofzablocki · 2016-12-31T10:10:50Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+        let uuid = NSUUID().uuidString
+        let jobCount = Int(ceil(Double(count) / Double(maxConcurrentJobs)))
+
+        let queueLabelPrefix = "io.realm.SwiftLintFramework.map.\(uuid)"


curious whether there is a reason you didn't use DispatchQueue.concurrentPerform here?

I didn't use it because I wasn't aware of it! Thanks for sharing 😊

The docs are non-existent, unfortunately: https://developer.apple.com/reference/dispatch/dispatchqueue/2016088-concurrentperform

But doing a bit of digging, the Dispatch Swift overlay seems to implement that function with dispatch_apply, which is well documented here: https://developer.apple.com/reference/dispatch/1452846-dispatch_apply_f

It seems to be exactly what I've reimplemented here, so I think I'll refactor to use that instead.

marcelofabri · 2016-12-31T16:57:39Z

Source/SwiftLintFramework/Extensions/File+Cache.swift

-private var _allDeclarationsByType = [String: [String]]()
-private var queueForRebuild = [Structure]()
+private struct RebuildQueue {
+    private let lock = NSLock()


I'm not sure here, but wouldn't using a queue be faster?

What makes you think that?

I had the impression that NSLocks are really expensive, but can't find any good references right now

I mean, locking in general has considerable overhead, but that goes for whether you're using NSLock or GCD queues...

Actually, some of the patterns here could avoid some locking by using lock-free data structures, since they're append-only and we end up sorting them afterwards, but I couldn't find good resources on lock-free data structures in Swift, so I've punted on that for now.

marcelofabri · 2016-12-31T17:01:48Z

Source/swiftlint/Commands/LintCommand.swift

@@ -16,6 +17,8 @@ struct LintCommand: CommandProtocol {
    let verb = "lint"
    let function = "Print lint warnings and errors (default command)"

+    private static let violationsAccumulatorQueue = DispatchQueue(label: "io.realm.swiftlint.violationsAccumulator")


I know probably no one will use multiple linters at once, but shouldn't this queue be per instance?

Done in 253e1f4.

marcelofabri · 2016-12-31T17:07:42Z

I got an exception in one of my runs:

Linting 'SchemaTests.swift' (29/41)
[2]    95692 illegal hardware instruction  
parallel_swiftlint/swiftlint lint

marcelofabri · 2016-12-31T17:08:21Z

And another one:

swiftlint(95787,0x7000043ed000) malloc: *** error for object 0x7f8e41915d18: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
[2]    95787 abort      parallel_swiftlint/swiftlint lint

marcelofabri · 2016-12-31T17:11:00Z

If it helps, I'm running on a 6-Core 2013 Mac Pro.

marcelofabri · 2016-12-31T17:25:10Z

Actually this just happens with the --benchmark option.

also remove parallelForEach from SwiftLintFramework's public interface since calling the GCD API is almost as concise and avoids polluting the public API. Finally, also remove the "fast" path for parallelMap since GCD should do the reasonable thing for us here.

jpsim · 2017-01-01T01:25:35Z

Yeah, I had neglected to make recording benchmarks thread-safe. Done in 253e1f4.

codecov-io · 2017-01-01T01:39:13Z

Current coverage is 82.41% (diff: 48.95%)

Merging #1103 into master will decrease coverage by 0.10%

@@             master      #1103   diff @@
==========================================
  Files           151        151          
  Lines          7209       7279    +70   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5949       5999    +50   
- Misses         1260       1280    +20   
  Partials          0          0

Powered by Codecov. Last update 6b2338c...3608cf4

marcelofabri

This 👏 is 👏 awesome! 👏

norio-nomura · 2017-01-01T08:35:31Z

I have gotten crashes twice on linting:
https://gist.github.com/norio-nomura/e8dc253954ec8299e1e647aad3d99398

norio-nomura · 2017-01-01T13:19:51Z

Source/swiftlint/Extensions/Configuration+CommandLine.swift

                }
-                autoreleasepool {


Is autoreleasepool removed?

Oops. Nice catch! Re-added in 3608cf4.

jpsim · 2017-01-01T18:12:51Z

Are those crashes reproducible?

jpsim · 2017-01-02T00:54:34Z

@norio-nomura what project(s) were you linting when you experienced these crashes?

norio-nomura · 2017-01-02T03:40:08Z

I experienced on Carthage. It happens rarely. But it happened first and second try with this PR.

jpsim · 2017-01-02T04:22:20Z

Hmmm, I ran this with Thread Sanitizer enabled against Carthage with all submodules recursively cloned 5 times in a row without hitting any issues. Xcode 8.2 with just a bare swiftlint command, no extra configuration files or CLI arguments.

The only change I've made was to enable all opt-in rules:

--- a/Source/SwiftLintFramework/Models/Configuration.swift
+++ b/Source/SwiftLintFramework/Models/Configuration.swift
@@ -91,11 +91,7 @@ public struct Configuration: Equatable {
                 return whitelistRules.contains(type(of: rule).description.identifier)
             }
         } else {
-            rules = configuredRules.filter { rule in
-                let id = type(of: rule).description.identifier
-                if validDisabledRules.contains(id) { return false }
-                return optInRules.contains(id) || !(rule is OptInRule)
-            }
+            rules = configuredRules
         }

norio-nomura · 2017-01-02T08:37:25Z

Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift

+                result.append(jobIndexAndResults)
+            }
+        }
+        return result.sorted { $0.0 < $1.0 }.flatMap { $0.1 }


Is the flatMap correct?

No, should've been changed to map in 8774a92. Fixed in 53d0cf6. Thanks!

norio-nomura · 2017-01-02T08:46:55Z

Source/SwiftLintFramework/Extensions/File+Cache.swift

        }
+        lock.unlock()
        let value = factory(file)


factory() invocation might be happen in parallel for same key file because it unlocked() while running factory().

That shouldn't be a problem when factory is already thread-safe. But anyway, I've refactored to perform the factory while locked in 8906da6, to make sure this keeps working if factory is ever not thread-safe.

Thanks for change.

I thought that it should be locked for functionality of Cache even if factory would be already thread-safe. Because cached contents possibly be reference type, and previous implementation could return difference instance for same key.

* master: use first(where:) instead of for loop in RulesCommand.swift remove unnecessary imports from AutoCorrectCommand.swift Typo in changelog Remove unnecessary import Avoid duplicating default value Update number_separator configuration in SwiftLint number_separator can be configured with a min length Enable more opt-in rules in SwiftLint Fix out of range exception in AttributesRule Fix trailing_comma crash

jpsim · 2017-01-02T18:08:23Z

I appreciate everyone's reviews on this! Thanks @norio-nomura, @marcelofabri and special guest @krzysztofzablocki 🙏.

I think I'll be merging this despite @norio-nomura's crashes, since neither of us appear to be able to reproduce them now.

jpsim added 5 commits December 30, 2016 23:07

make Cache thread-safe

8a4671e

and make some of its members private instead of fileprivate

extend Array with parallelMap, parallelFlatMap and parallelForEach

28813fc

parallelize getting violations for a single file

d82ca68

parallelize linting multiple files

f24ceac

add changelog entries for parallelization and thread safety

914aeb1

jpsim requested review from norio-nomura and marcelofabri December 31, 2016 07:39

krzysztofzablocki reviewed Dec 31, 2016

View reviewed changes

marcelofabri reviewed Dec 31, 2016

View reviewed changes

jpsim added 2 commits December 31, 2016 17:06

make recording benchmarks thread-safe

253e1f4

marcelofabri approved these changes Jan 1, 2017

View reviewed changes

norio-nomura reviewed Jan 1, 2017

View reviewed changes

re-add autoreleasepool around visitorBlock invocation

3608cf4

norio-nomura reviewed Jan 2, 2017

View reviewed changes

fix incorrect flatMap usage to use map instead

53d0cf6

jpsim added 2 commits January 2, 2017 09:52

perform Cache factory while locked

8906da6

jpsim merged commit 2ff0348 into master Jan 2, 2017

jpsim deleted the jp-parallelize branch January 2, 2017 18:12

marcelofabri mentioned this pull request Jan 2, 2017

Parallel linting #1077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make getting style violations thread-safe and parallelize linting #1103

Make getting style violations thread-safe and parallelize linting #1103

jpsim commented Dec 31, 2016

SwiftLintBot commented Dec 31, 2016 •

edited

Loading

krzysztofzablocki Dec 31, 2016

jpsim Jan 1, 2017

marcelofabri Dec 31, 2016

jpsim Jan 1, 2017

marcelofabri Jan 1, 2017

jpsim Jan 1, 2017

marcelofabri Dec 31, 2016

jpsim Jan 1, 2017

marcelofabri commented Dec 31, 2016 •

edited

Loading

marcelofabri commented Dec 31, 2016

marcelofabri commented Dec 31, 2016

marcelofabri commented Dec 31, 2016

jpsim commented Jan 1, 2017

codecov-io commented Jan 1, 2017 •

edited

Loading

marcelofabri left a comment

norio-nomura commented Jan 1, 2017

norio-nomura Jan 1, 2017 •

edited

Loading

jpsim Jan 1, 2017

jpsim commented Jan 1, 2017

jpsim commented Jan 2, 2017

norio-nomura commented Jan 2, 2017

jpsim commented Jan 2, 2017

norio-nomura Jan 2, 2017

jpsim Jan 2, 2017

norio-nomura Jan 2, 2017

jpsim Jan 2, 2017

norio-nomura Jan 3, 2017

jpsim commented Jan 2, 2017

Make getting style violations thread-safe and parallelize linting #1103

Make getting style violations thread-safe and parallelize linting #1103

Conversation

jpsim commented Dec 31, 2016

SwiftLintBot commented Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Dec 31, 2016 • edited Loading

marcelofabri commented Dec 31, 2016

marcelofabri commented Dec 31, 2016

marcelofabri commented Dec 31, 2016

jpsim commented Jan 1, 2017

codecov-io commented Jan 1, 2017 • edited Loading

Current coverage is 82.41% (diff: 48.95%)

marcelofabri left a comment

Choose a reason for hiding this comment

norio-nomura commented Jan 1, 2017

norio-nomura Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Jan 1, 2017

jpsim commented Jan 2, 2017

norio-nomura commented Jan 2, 2017

jpsim commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Jan 2, 2017

SwiftLintBot commented Dec 31, 2016 •

edited

Loading

marcelofabri commented Dec 31, 2016 •

edited

Loading

codecov-io commented Jan 1, 2017 •

edited

Loading

norio-nomura Jan 1, 2017 •

edited

Loading