Refactor generation of SHA256 checksums

[mattt]

Improves performance and convenience of SHA256 checksum generation.

Motivation:

While working on #3023, dependency resolution with package registries was significantly slower than I expected. After a bit of profiling, I determined that a call to SHA256().hash for the downloaded Zip archive of each package release was responsible for the slowdown. Switching to the CryptoKit implementation of SHA256 created improved end-to-end performance from 70s to 15s. (See #3023 (comment))

Modifications:

This PR starts by creating a sha256Checksum property in an extension on ByteString that provides a lowercase hexadecimal representation of the SHA256 hash, using CryptoKit when available (0bd6a54).

Next, it refactors existing usage of SHA256 to use this new property (8512a88, 85fb8da, c92682c).

Finally, it changes the Workspace initializer to upgrade the provided checksumAlgorithm parameter to CryptoKitSHA256 when CryptoKit is available and a default SHA256 value is provided (246cadb).

Result:

With these changes, Swift Package Manager should perform certain tasks somewhat faster. In particular, I believe this should significantly improve the performance of working with binary artifacts, which currently uses the slower, native TSCBasic SHA256 implementation.

This change will also be useful for #3023, which will rebase to consolidate its existing conditional use of CryptoKit.

[mattt]

@swift-ci please smoke test

[tomerd]

this is great!

[tomerd]

looks like the new integration tests from #3088 are failing unrelated to this PR change (reproduced the same failure locally on main branch). cc @abertelrud @friedbunny

[hartbit]

@mattt This looks great! But wouldn't it have made more sense to do this in TSCBasic so that other projects that use TSCBasic could also benefit from a faster sha256 when CryptoKit is available? I think that was the idea initially.

[tomerd]

@mattt this looks great. we are going into a merge freeze for non-critical bug fixes until after the 5.4 branch is created on 1/7 (see https://forums.swift.org/t/swift-5-4-release-process/), so will merge it then

[abertelrud]

Would it make sense to return the actual data here, and then the call site can convert it to hex, or base-whatever, etc? The representation seems orthogonal to the computation.

[mattt]

Looking at the existing call sites, we only call SHA256().hash(bytes) to produce a hexadecimal representation. Returning ByteString here would be more flexible, but that may be unnecessary. Really, it's a matter of taste; I'm happy with any solution that makes it easy to opt-in to a faster hash function when available.

[abertelrud]

It's fine to leave this in this PR. I just have a general preference for separating out functionality into orthogonal parts, but since this is already an extension ByteString, it's fine to leave as is.

ByteString came about back before String had a UTF-8 backing, so performance was a problem in going between UTF-8 data and String. My sense is that we could probably transition to String here over time, at which point we might want to revisit this.

[mattt]

A big 👍 for transitioning away from ByteString at some point. When I started working on the codebase for registry support, I found the use of TSC / SPM-specific APIs instead of Swift Standard Library and Foundation (and now System) counterparts to be a frequent point of frustration. Many of these APIs were necessary at the time (e.g. JSON before Codable), but have now become technical debt.

[tomerd]

@swift-ci please smoke test macOS Platform

[tomerd]

@mattt could you plz look at resolving the conflicts with this one

[mattt]

@swift-ci please smoke test

[tomerd]

this feels a bit hacky... could we change the ctor argument checksumAlgorithm: HashAlgorithm = SHA256() to checksumAlgorithm: HashAlgorithm? = nil and create default instead?

[mattt]

I think this version has clearer semantics; the default is SHA256, not nil, and we're upgrading to a faster alternative when available. I also value the consistency with the preceding archiver parameter in the Workspace initializer and the checksumAlgorithm parameter MockWorkspace. But I yield to your preference on this. Here's the equivalent using a nil argument:

public init(
        dataPath: AbsolutePath,
        editablesPath: AbsolutePath,
        pinsFile: AbsolutePath,
        manifestLoader: ManifestLoaderProtocol,
        repositoryManager: RepositoryManager? = nil,
        currentToolsVersion: ToolsVersion = ToolsVersion.currentToolsVersion,
        toolsVersionLoader: ToolsVersionLoaderProtocol = ToolsVersionLoader(),
        delegate: WorkspaceDelegate? = nil,
        config: Workspace.Configuration = Workspace.Configuration(),
        fileSystem: FileSystem = localFileSystem,
        repositoryProvider: RepositoryProvider = GitRepositoryProvider(),
        downloader: Downloader = FoundationDownloader(),
        netrcFilePath: AbsolutePath? = nil,
        archiver: Archiver = ZipArchiver(),
        checksumAlgorithm: HashAlgorithm? = nil,
    ) {
       // ...

        var checksumAlgorithm = checksumAlgorithm ?? SHA256()
        #if canImport(CryptoKit)
        if #available(macOS 10.15, *) {
            checksumAlgorithm = CryptoKitSHA256()
        }
        #endif
        self.checksumAlgorithm = checksumAlgorithm

[tomerd]

my point was to try and avoid allocating memory twice. since this is the workspace and it usually only initialized once I guess this is not very critical. lets take the PR as is and we can modify it later if we decide to

[tomerd]

@swift-ci please smoke test