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

Add multiple cache storages feature #156

Merged
merged 32 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
6d35c14
Add multiple cache storages feature
ikesyo Nov 7, 2024
5953d6c
Pass storages to `CacheSystem.cacheFrameworks`
ikesyo Nov 8, 2024
a24f7c9
Refactor to remove isProducingCacheEnabled
ikesyo Nov 8, 2024
d7bd9ff
Refactor to remove isConsumingCacheEnabled
ikesyo Nov 8, 2024
e7b20c5
Improve logging
ikesyo Nov 8, 2024
0f1c98a
debugging
ikesyo Nov 8, 2024
8750153
Add test case for storages cache mode
ikesyo Nov 11, 2024
8c7c16e
Update `build-pipeline.md` for `.storages` cache mode
ikesyo Nov 11, 2024
f6cbef1
debugging
ikesyo Nov 11, 2024
413bb12
Add StorageConfig struct
ikesyo Nov 12, 2024
c3d9efa
Address naming convention issues
ikesyo Nov 12, 2024
25f5fcb
Replace `case storage` with static func
ikesyo Nov 12, 2024
82ec106
Replace `case disabled` with static func
ikesyo Nov 12, 2024
086970f
Merge branch 'main' into multiple-cache-storages
ikesyo Nov 12, 2024
7434f6b
Rename to LocalDiskCacheStorage
ikesyo Nov 12, 2024
a451614
Introduce ProjectCacheStorage
ikesyo Nov 12, 2024
c22ed6f
Replace `case project` with static func (using ProjectCacheStorage)
ikesyo Nov 12, 2024
0006fbb
Introduce CachePolicy and replace CacheMode with an array of CachePolicy
ikesyo Nov 12, 2024
4d8abab
Update documentation
ikesyo Nov 12, 2024
76ae5af
Avoid direct usages of `type(of: storage)`
ikesyo Nov 12, 2024
ff1d483
Make LocalDiskCacheStorage and ProjectCacheStorage internal
ikesyo Nov 14, 2024
4712f89
Use some over any for arguments
ikesyo Nov 14, 2024
3d333e6
[CI] macos-15 image is required to use Xcode 16
ikesyo Nov 14, 2024
0c81a2e
Add info log when a cache is not found when restoring as well as `.su…
ikesyo Nov 14, 2024
d71053b
Fix format
ikesyo Nov 14, 2024
ed52625
Use some over any for arguments
ikesyo Nov 14, 2024
4de26b5
Add inline comment
ikesyo Nov 14, 2024
bced0e2
Reduce duplication
ikesyo Nov 14, 2024
dfe443c
Revert "Reduce duplication"
ikesyo Nov 14, 2024
acdd251
Reduce duplication
ikesyo Nov 14, 2024
afafa8d
Revert "debugging"
ikesyo Nov 14, 2024
2720cde
Add `[Runner.Options.CachePolicy].disabled`
ikesyo Nov 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions Sources/ScipioKit/Producer/Cache/CacheSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ struct CacheSystem: Sendable {
static let defaultParalellNumber = 8
private let pinsStore: PinsStore
private let outputDirectory: URL
private let storage: (any CacheStorage)?
Copy link
Collaborator Author

@ikesyo ikesyo Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the introduction of cache policies, storages may be different on when saving caches and when restoring caches. So removed the property and passes by arguments instead.

private let fileSystem: any FileSystem

struct CacheTarget: Hashable, Sendable {
Expand Down Expand Up @@ -139,18 +138,23 @@ struct CacheSystem: Sendable {
init(
pinsStore: PinsStore,
outputDirectory: URL,
storage: (any CacheStorage)?,
fileSystem: any FileSystem = localFileSystem
) {
self.pinsStore = pinsStore
self.outputDirectory = outputDirectory
self.storage = storage
self.fileSystem = fileSystem
}

func cacheFrameworks(_ targets: Set<CacheTarget>) async {
let chunked = targets.chunks(ofCount: storage?.parallelNumber ?? CacheSystem.defaultParalellNumber)
func cacheFrameworks(_ targets: Set<CacheTarget>, to storages: [any CacheStorage]) async {
for storage in storages {
await cacheFrameworks(targets, to: storage)
}
}

private func cacheFrameworks(_ targets: Set<CacheTarget>, to storage: any CacheStorage) async {
ikesyo marked this conversation as resolved.
Show resolved Hide resolved
let chunked = targets.chunks(ofCount: storage.parallelNumber ?? CacheSystem.defaultParalellNumber)

let storageName = storage.displayName
for chunk in chunked {
await withTaskGroup(of: Void.self) { group in
for target in chunk {
Expand All @@ -159,10 +163,10 @@ struct CacheSystem: Sendable {
let frameworkPath = outputDirectory.appendingPathComponent(frameworkName)
do {
logger.info(
"🚀 Cache \(frameworkName) to cache storage",
"🚀 Cache \(frameworkName) to cache storage: \(storageName)",
metadata: .color(.green)
)
try await cacheFramework(target, at: frameworkPath)
try await cacheFramework(target, at: frameworkPath, to: storage)
} catch {
logger.warning("⚠️ Can't create caches for \(frameworkPath.path)")
}
Expand All @@ -173,10 +177,10 @@ struct CacheSystem: Sendable {
}
}

private func cacheFramework(_ target: CacheTarget, at frameworkPath: URL) async throws {
private func cacheFramework(_ target: CacheTarget, at frameworkPath: URL, to storage: any CacheStorage) async throws {
let cacheKey = try await calculateCacheKey(of: target)

try await storage?.cacheFramework(frameworkPath, for: cacheKey)
try await storage.cacheFramework(frameworkPath, for: cacheKey)
}

func generateVersionFile(for target: CacheTarget) async throws {
Expand Down Expand Up @@ -210,8 +214,8 @@ struct CacheSystem: Sendable {
case failed(LocalizedError?)
case noCache
}
func restoreCacheIfPossible(target: CacheTarget) async -> RestoreResult {
guard let storage = storage else { return .noCache }

func restoreCacheIfPossible(target: CacheTarget, storage: any CacheStorage) async -> RestoreResult {
ikesyo marked this conversation as resolved.
Show resolved Hide resolved
do {
let cacheKey = try await calculateCacheKey(of: target)
if try await storage.existsValidCache(for: cacheKey) {
Expand All @@ -225,12 +229,6 @@ struct CacheSystem: Sendable {
}
}

private func fetchArtifacts(target: CacheTarget, to destination: URL) async throws {
Copy link
Collaborator Author

@ikesyo ikesyo Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was unused.

guard let storage = storage else { return }
let cacheKey = try await calculateCacheKey(of: target)
try await storage.fetchArtifacts(for: cacheKey, to: destination)
}

func calculateCacheKey(of target: CacheTarget) async throws -> SwiftPMCacheKey {
let targetName = target.buildProduct.target.name
let pin = try retrievePin(package: target.buildProduct.package)
Expand All @@ -247,7 +245,10 @@ struct CacheSystem: Sendable {
buildOptions: buildOptions,
clangVersion: clangVersion,
xcodeVersion: xcodeVersion,
scipioVersion: currentScipioVersion
// Making the cache key compatible with 0.24.0 temporarily for easier debugging.
//
// TODO: revert this before merging
scipioVersion: "578ce98d236e79dad3e473cb11153e867be07174"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the feature, we're better to add --disable-scipio-version-management option for this purpose.
It's useful for the development phase.

Copy link
Collaborator Author

@ikesyo ikesyo Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling when developing is not a solution, because if excluding scipioVersion from cache keys, those are not compatible with the cache keys generated by release-versioned Scipio.

Ideally we should introduce some kind of Scipio's cache versioning and increment it when there are some incompatible changes to built frameworks (but it'll make Scipio's development and review process a bit complicated).

)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import ScipioStorage
import PackageGraph
import TSCBasic

public struct LocalCacheStorage: CacheStorage {
public struct LocalDiskCacheStorage: CacheStorage {
private let fileSystem: any FileSystem

public var parallelNumber: Int? { nil }
Expand Down
10 changes: 10 additions & 0 deletions Sources/ScipioKit/Producer/Cache/ProjectCacheStorage.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import Foundation
import ScipioStorage

/// The pseudo cache storage for "project cache policy", which treats built frameworks under the project's output directory (e.g. `XCFrameworks`)
/// as valid caches but does not saving / restoring anything.
public struct ProjectCacheStorage: CacheStorage {
public func existsValidCache(for cacheKey: some ScipioStorage.CacheKey) async throws -> Bool { false }
public func fetchArtifacts(for cacheKey: some ScipioStorage.CacheKey, to destinationDir: URL) async throws {}
public func cacheFramework(_ frameworkPath: URL, for cacheKey: some ScipioStorage.CacheKey) async throws {}
}
9 changes: 9 additions & 0 deletions Sources/ScipioKit/Producer/CacheStorage+DisplayName.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import ScipioStorage

extension CacheStorage {
/// The display name of the cache storage used for logging purpose
var displayName: String {
// TODO: Define the property as CacheStorage's requirement in scipio-cache-storage
"\(type(of: self))"
}
}
160 changes: 101 additions & 59 deletions Sources/ScipioKit/Producer/FrameworkProducer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,31 +10,15 @@ struct FrameworkProducer {
private let descriptionPackage: DescriptionPackage
private let baseBuildOptions: BuildOptions
private let buildOptionsMatrix: [String: BuildOptions]
private let cacheMode: Runner.Options.CacheMode
private let cachePolicies: [Runner.Options.CachePolicy]
private let overwrite: Bool
private let outputDir: URL
private let fileSystem: any FileSystem
private let toolchainEnvironment: [String: String]?

private var cacheStorage: (any CacheStorage)? {
switch cacheMode {
case .disabled, .project: return nil
case .storage(let storage, _): return storage
}
}

private var isProducingCacheEnabled: Bool {
switch cacheMode {
case .disabled: return false
case .project: return true
case .storage(_, let actors):
return actors.contains(.producer)
}
}

private var shouldGenerateVersionFile: Bool {
// cacheMode is not disabled
if case .disabled = cacheMode {
// cache is not disabled
guard !cachePolicies.isEmpty else {
return false
}

Expand All @@ -49,7 +33,7 @@ struct FrameworkProducer {
descriptionPackage: DescriptionPackage,
buildOptions: BuildOptions,
buildOptionsMatrix: [String: BuildOptions],
cacheMode: Runner.Options.CacheMode,
cachePolicies: [Runner.Options.CachePolicy],
overwrite: Bool,
outputDir: URL,
toolchainEnvironment: [String: String]? = nil,
Expand All @@ -58,7 +42,7 @@ struct FrameworkProducer {
self.descriptionPackage = descriptionPackage
self.baseBuildOptions = buildOptions
self.buildOptionsMatrix = buildOptionsMatrix
self.cacheMode = cacheMode
self.cachePolicies = cachePolicies
self.overwrite = overwrite
self.outputDir = outputDir
self.toolchainEnvironment = toolchainEnvironment
Expand Down Expand Up @@ -108,12 +92,14 @@ struct FrameworkProducer {
let pinsStore = try descriptionPackage.workspace.pinsStore.load()
let cacheSystem = CacheSystem(
pinsStore: pinsStore,
outputDirectory: outputDir,
storage: cacheStorage
outputDirectory: outputDir
)

let targetsToBuild: OrderedSet<CacheSystem.CacheTarget>
if cacheMode.isConsumingCacheEnabled {
if cachePolicies.isEmpty {
// no-op because cache is disabled
targetsToBuild = allTargets
} else {
let targets = Set(allTargets)

// Validate the existing frameworks in `outputDir` before restoration
Expand All @@ -122,16 +108,20 @@ struct FrameworkProducer {
cacheSystem: cacheSystem
)

let restored = await restoreAllAvailableCaches(
availableTargets: targets.subtracting(valid),
cacheSystem: cacheSystem
)

targetsToBuild = allTargets
.subtracting(valid)
.subtracting(restored)
} else {
targetsToBuild = allTargets
let storagesWithConsumer = cachePolicies.storages(for: .consumer)
if storagesWithConsumer.isEmpty {
// no-op
targetsToBuild = allTargets.subtracting(valid)
} else {
let restored = await restoreAllAvailableCachesIfNeeded(
availableTargets: targets.subtracting(valid),
to: storagesWithConsumer,
cacheSystem: cacheSystem
)
targetsToBuild = allTargets
.subtracting(valid)
.subtracting(restored)
}
}

for target in targetsToBuild {
Expand All @@ -142,9 +132,7 @@ struct FrameworkProducer {
)
}

if isProducingCacheEnabled {
await cacheSystem.cacheFrameworks(Set(targetsToBuild))
}
await cacheFrameworksIfNeeded(Set(targetsToBuild), cacheSystem: cacheSystem)

if shouldGenerateVersionFile {
// Versionfiles should be generate for all targets
Expand All @@ -167,13 +155,20 @@ struct FrameworkProducer {
group.addTask { [outputDir, fileSystem] in
do {
let product = target.buildProduct
let outputPath = outputDir.appendingPathComponent(product.frameworkName)
let frameworkName = product.frameworkName
let outputPath = outputDir.appendingPathComponent(frameworkName)
let exists = fileSystem.exists(outputPath.absolutePath)
guard exists else { return nil }

let expectedCacheKey = try await cacheSystem.calculateCacheKey(of: target)
let isValidCache = await cacheSystem.existsValidCache(cacheKey: expectedCacheKey)
guard isValidCache else { return nil }
guard isValidCache else {
logger.warning("⚠️ Existing \(frameworkName) is outdated.", metadata: .color(.yellow))
logger.info("🗑️ Delete \(frameworkName)", metadata: .color(.red))
try fileSystem.removeFileTree(outputPath.absolutePath)

return nil
}

let expectedCacheKeyHash = try expectedCacheKey.calculateChecksum()
logger.info(
Expand All @@ -194,11 +189,58 @@ struct FrameworkProducer {
return validFrameworks
}

private func restoreAllAvailableCaches(
private func restoreAllAvailableCachesIfNeeded(
availableTargets: Set<CacheSystem.CacheTarget>,
to storages: [any CacheStorage],
cacheSystem: CacheSystem
) async -> Set<CacheSystem.CacheTarget> {
var remainingTargets = availableTargets
var restored: Set<CacheSystem.CacheTarget> = []

for index in storages.indices {
let storage = storages[index]

let logSuffix = "[\(index)] \(storage.displayName)"
if index == storages.startIndex {
logger.info(
"▶️ Starting restoration with cache storage: \(logSuffix)",
metadata: .color(.green)
)
} else {
logger.info(
"⏭️ Falling back to next cache storage: \(logSuffix)",
metadata: .color(.green)
)
}

let restoredPerStorage = await restoreCaches(
for: remainingTargets,
from: storage,
cacheSystem: cacheSystem
)
restored.formUnion(restoredPerStorage)

logger.info(
"⏸️ Restoration finished with cache storage: \(logSuffix)",
metadata: .color(.green)
)

remainingTargets.subtract(restoredPerStorage)
if remainingTargets.isEmpty {
ikesyo marked this conversation as resolved.
Show resolved Hide resolved
break
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if all frameworks are successfully restored, we don't need to proceed to next cache storage.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[not mandatory] it's kindly to comment this

}

logger.info("⏹️ Restoration finished", metadata: .color(.green))
return restored
}

private func restoreCaches(
for targets: Set<CacheSystem.CacheTarget>,
from cacheStorage: any CacheStorage,
cacheSystem: CacheSystem
) async -> Set<CacheSystem.CacheTarget> {
let chunked = availableTargets.chunks(ofCount: cacheStorage?.parallelNumber ?? CacheSystem.defaultParalellNumber)
let chunked = targets.chunks(ofCount: cacheStorage.parallelNumber ?? CacheSystem.defaultParalellNumber)

var restored: Set<CacheSystem.CacheTarget> = []
for chunk in chunked {
Expand All @@ -209,7 +251,8 @@ struct FrameworkProducer {
do {
let restored = try await restorer.restore(
target: target,
cacheSystem: cacheSystem
cacheSystem: cacheSystem,
cacheStorage: cacheStorage
)
return restored ? target : nil
} catch {
Expand All @@ -233,23 +276,16 @@ struct FrameworkProducer {
// Return true if pre-built artifact is available (already existing or restored from cache)
func restore(
target: CacheSystem.CacheTarget,
cacheSystem: CacheSystem
cacheSystem: CacheSystem,
cacheStorage: any CacheStorage
) async throws -> Bool {
let product = target.buildProduct
let frameworkName = product.frameworkName
let outputPath = outputDir.appendingPathComponent(frameworkName)
let exists = fileSystem.exists(outputPath.absolutePath)

let expectedCacheKey = try await cacheSystem.calculateCacheKey(of: target)
let expectedCacheKeyHash = try expectedCacheKey.calculateChecksum()

if exists {
logger.warning("⚠️ Existing \(frameworkName) is outdated.", metadata: .color(.yellow))
logger.info("🗑️ Delete \(frameworkName)", metadata: .color(.red))
try fileSystem.removeFileTree(outputPath.absolutePath)
}

let restoreResult = await cacheSystem.restoreCacheIfPossible(target: target)
let restoreResult = await cacheSystem.restoreCacheIfPossible(target: target, storage: cacheStorage)
switch restoreResult {
case .succeeded:
logger.info("✅ Restore \(frameworkName) (\(expectedCacheKeyHash)) from cache storage.", metadata: .color(.green))
Expand Down Expand Up @@ -303,6 +339,13 @@ struct FrameworkProducer {
return []
}

private func cacheFrameworksIfNeeded(_ targets: Set<CacheSystem.CacheTarget>, cacheSystem: CacheSystem) async {
let storagesWithProducer = cachePolicies.storages(for: .producer)
if !storagesWithProducer.isEmpty {
await cacheSystem.cacheFrameworks(targets, to: storagesWithProducer)
}
Comment on lines +345 to +348
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] I'm concerned that the way storages are handled is different for consumers and producers.

For consumers, storages are evaluated in order and the top ones are used.
On the other hand, producers try to write to all declared storages at the same time.

Is this difference in behavior intentional?

Copy link
Collaborator Author

@ikesyo ikesyo Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional. If we don't cache built frameworks to 2nd storages and later, the storages will not be able to restore the frameworks so saving to all storages will be required I think.

}

private func generateVersionFile(for target: CacheSystem.CacheTarget, using cacheSystem: CacheSystem) async {
do {
try await cacheSystem.generateVersionFile(for: target)
Expand All @@ -312,13 +355,12 @@ struct FrameworkProducer {
}
}

extension Runner.Options.CacheMode {
fileprivate var isConsumingCacheEnabled: Bool {
switch self {
case .disabled: return false
case .project: return true
case .storage(_, let actors):
return actors.contains(.consumer)
extension [Runner.Options.CachePolicy] {
fileprivate func storages(for actor: Runner.Options.CachePolicy.CacheActorKind) -> [any CacheStorage] {
reduce(into: []) { result, cachePolicy in
if cachePolicy.actors.contains(actor) {
result.append(cachePolicy.storage)
}
}
}
}
Loading