Skip to content

Commit 3c5d9fb

Browse files
committed
Fix and improve parallel mode
Parallel mode was crashing with bad memory access due to data races accessing `Frontend.configurationLoader`, more specifically its `cache`. After serializing access (initially with an `NSLock`), I observed that despite not crashing anymore, the performance was surprisingly *worst* than in single threaded mode. That led me down a road of investigating why this was happening, and after some profiling I discovered that `Rule`'s `nameCache` was the main source of contention - causing around 30% of total spent time waiting for locks (`ulock_wait`) on my tests. After replacing the synchronization mechanism on `nameCache` with a more efficient `os_unfair_lock_t` (`pthread_mutex_t` in Linux), the contention dropped significantly, and parallel mode now outperformed single threaded mode as expected. As a bonus, these changes also improved single threaded mode performance as well, due to the reduced overhead of using a lock vs a queue. I then used the same `Lock` approach to serialize access to `Frontend.configurationLoader` which increased the performance gap even further. After these improvements, I was able to obtain quite significant performance gains: - serial (non optimized) vs parallel (optimized): ~5.4x (13.5s vs 74s) - serial (optimized) vs serial (non optimized): ~1.6x (44s vs 74s) - serial (optimized) vs parallel (optimized): ~3.2x (13.5s vs 44s) Tests were made on my `MacBookPro16,1` (8-core [email protected]), on a project with 2135 Swift files, compiling `swift-format` in Release mode. ## Changes - Add new `Lock` utility to `SwiftFormatCore` which exposes `os_unfair_lock_t` on macOS and `pthread_mutex_t` as the fallback for Linux (or for recursive locks). - Use the new `Lock` to serialize access to `Frontend.configurationLoader`. - Use the new `Lock` to serialize access to `Rule`'s `nameCache` (replacing `nameCacheQueue`)
1 parent e3a4b21 commit 3c5d9fb

File tree

3 files changed

+174
-12
lines changed

3 files changed

+174
-12
lines changed

Sources/SwiftFormatCore/Rule.swift

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,26 +28,22 @@ public protocol Rule {
2828
}
2929

3030
fileprivate var nameCache = [ObjectIdentifier: String]()
31-
fileprivate var nameCacheQueue = DispatchQueue(
32-
label: "com.apple.SwiftFormat.NameCache", attributes: .concurrent)
31+
fileprivate let nameCacheLock = Lock.make()
3332

3433
extension Rule {
3534
/// By default, the `ruleName` is just the name of the implementing rule class.
3635
public static var ruleName: String {
3736
let identifier = ObjectIdentifier(self)
38-
let cachedName = nameCacheQueue.sync {
39-
nameCache[identifier]
40-
}
4137

42-
if let cachedName = cachedName {
38+
nameCacheLock.lock()
39+
defer { nameCacheLock.unlock() }
40+
41+
if let cachedName = nameCache[identifier] {
4342
return cachedName
4443
}
4544

4645
let name = String("\(self)".split(separator: ".").last!)
47-
nameCacheQueue.async(flags: .barrier) {
48-
nameCache[identifier] = name
49-
}
50-
46+
nameCache[identifier] = name
5147
return name
5248
}
5349
}
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
// Copied and modified from:
14+
// https://github.com/ReactiveCocoa/ReactiveSwift/blob/master/Sources/Atomic.swift 🙏
15+
//
16+
// Copyright (c) 2012 - 2016, GitHub, Inc. All rights reserved.
17+
//
18+
// Permission is hereby granted, free of charge, to any person obtaining a copy of
19+
// this software and associated documentation files (the "Software"), to deal in
20+
// the Software without restriction, including without limitation the rights to
21+
// use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
22+
// the Software, and to permit persons to whom the Software is furnished to do so,
23+
// subject to the following conditions:
24+
//
25+
// The above copyright notice and this permission notice shall be included in all
26+
// copies or substantial portions of the Software.
27+
//
28+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
29+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
30+
// FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
31+
// COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
32+
// IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
33+
// CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
34+
35+
import Foundation
36+
#if os(macOS)
37+
import MachO
38+
#endif
39+
40+
/// `Lock` exposes `os_unfair_lock` on supported platforms, with `pthread_mutex_t`
41+
/// as the fallback (or for recursive locks).
42+
public class Lock {
43+
44+
#if os(macOS)
45+
@available(macOS 10.12, *)
46+
final class UnfairLock: Lock {
47+
private let _lock: os_unfair_lock_t
48+
49+
override init() {
50+
_lock = .allocate(capacity: 1)
51+
_lock.initialize(to: os_unfair_lock())
52+
super.init()
53+
}
54+
55+
override func lock() {
56+
os_unfair_lock_lock(_lock)
57+
}
58+
59+
override func unlock() {
60+
os_unfair_lock_unlock(_lock)
61+
}
62+
63+
override func `try`() -> Bool {
64+
return os_unfair_lock_trylock(_lock)
65+
}
66+
67+
deinit {
68+
_lock.deinitialize(count: 1)
69+
_lock.deallocate()
70+
}
71+
}
72+
#endif
73+
74+
final class PthreadLock: Lock {
75+
private let _lock: UnsafeMutablePointer<pthread_mutex_t>
76+
77+
init(recursive: Bool = false) {
78+
_lock = .allocate(capacity: 1)
79+
_lock.initialize(to: pthread_mutex_t())
80+
81+
let attr = UnsafeMutablePointer<pthread_mutexattr_t>.allocate(capacity: 1)
82+
attr.initialize(to: pthread_mutexattr_t())
83+
pthread_mutexattr_init(attr)
84+
85+
defer {
86+
pthread_mutexattr_destroy(attr)
87+
attr.deinitialize(count: 1)
88+
attr.deallocate()
89+
}
90+
91+
pthread_mutexattr_settype(
92+
attr, Int32(recursive ? PTHREAD_MUTEX_RECURSIVE : PTHREAD_MUTEX_ERRORCHECK))
93+
94+
let status = pthread_mutex_init(_lock, attr)
95+
assert(status == 0, "Unexpected pthread mutex error code: \(status)")
96+
97+
super.init()
98+
}
99+
100+
override func lock() {
101+
let status = pthread_mutex_lock(_lock)
102+
assert(status == 0, "Unexpected pthread mutex error code: \(status)")
103+
}
104+
105+
override func unlock() {
106+
let status = pthread_mutex_unlock(_lock)
107+
assert(status == 0, "Unexpected pthread mutex error code: \(status)")
108+
}
109+
110+
override func `try`() -> Bool {
111+
let status = pthread_mutex_trylock(_lock)
112+
switch status {
113+
case 0:
114+
return true
115+
case EBUSY, EAGAIN, EDEADLK:
116+
return false
117+
default:
118+
assertionFailure("Unexpected pthread mutex error code: \(status)")
119+
return false
120+
}
121+
}
122+
123+
deinit {
124+
let status = pthread_mutex_destroy(_lock)
125+
assert(status == 0, "Unexpected pthread mutex error code: \(status)")
126+
127+
_lock.deinitialize(count: 1)
128+
_lock.deallocate()
129+
}
130+
}
131+
132+
/// Return an instance of a `Lock`, according to API availability
133+
/// (`os_unfair_lock_t` or `pthread_mutex_t` based).
134+
///
135+
/// - returns: a `Lock` instance
136+
public static func make() -> Self {
137+
#if os(macOS)
138+
if #available(*, macOS 10.12) {
139+
return UnfairLock() as! Self
140+
}
141+
#endif
142+
143+
return PthreadLock() as! Self
144+
}
145+
146+
private init() {}
147+
148+
public func lock() { fatalError() }
149+
public func unlock() { fatalError() }
150+
public func `try`() -> Bool { fatalError() }
151+
}

Sources/swift-format/Frontend/Frontend.swift

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import Foundation
1414
import SwiftFormat
1515
import SwiftFormatConfiguration
16+
import SwiftFormatCore
1617
import SwiftSyntax
1718

1819
class Frontend {
@@ -78,6 +79,9 @@ class Frontend {
7879
/// Indicates whether any errors were emitted during execution.
7980
final var errorsWereEmitted: Bool { diagnosticEngine.hasErrors }
8081

82+
/// Lock to serialize access to shared state in parallel mode.
83+
final let lock = Lock.make()
84+
8185
/// Creates a new frontend with the given options.
8286
///
8387
/// - Parameter lintFormatOptions: Options that apply during formatting or linting.
@@ -181,7 +185,7 @@ class Frontend {
181185
// loaded. (Do not try to fall back to a path inferred from the source file path.)
182186
if let configurationFilePath = configurationFilePath {
183187
do {
184-
return try configurationLoader.configuration(atPath: configurationFilePath)
188+
return try lock.sync { try configurationLoader.configuration(atPath: configurationFilePath) }
185189
} catch {
186190
diagnosticEngine.diagnose(
187191
Diagnostic.Message(.error, "Unable to read configuration: \(error.localizedDescription)"))
@@ -194,7 +198,8 @@ class Frontend {
194198
if let swiftFilePath = swiftFilePath {
195199
do {
196200
if let configuration =
197-
try configurationLoader.configuration(forSwiftFileAtPath: swiftFilePath)
201+
try lock.sync(
202+
work: { try configurationLoader.configuration(forSwiftFileAtPath: swiftFilePath) })
198203
{
199204
return configuration
200205
}
@@ -213,3 +218,13 @@ class Frontend {
213218
return Configuration()
214219
}
215220
}
221+
222+
private extension Lock {
223+
224+
@discardableResult
225+
func sync<R>(work: () throws -> R) rethrows -> R {
226+
lock()
227+
defer { unlock() }
228+
return try work()
229+
}
230+
}

0 commit comments

Comments
 (0)