Paralellize retrieving resolved packages #8220

fortmarek · 2025-01-14T22:23:25Z

Paralellize retrieving resolved packages

Motivation:

This PR brings back the parallelization reverted here.

As raised in the revertal, checkouts and registry downloads mutate the WorkspaceState – and running these operations in parallel can lead to a crash since the WorkspaceState is not thread-safe.

Modifications:

Based on this suggestion, I'm making WorkspaceState an actor to make access to it thread-safe.

Additionally, I turned ManagedDependencies into a struct to ensure its thread-safety, too. There are two methods of this struct that mutate self – add and remove. I changed this to return a new copy of ManagedDependencies instead and add counterpart methods in WorkspaceState where the mutation is done in-place. Since ManagedDependencies is a Collection, making it into an actor because of two mutable methods didn't feel like the right fit. However, let me know what you think about it.

I added comments to the key modifications to make it easier to find the more interesting modifications since there's a lot of noise since I had to add loads of async and await. You can find these comments below.

Result:

Retrieving resolved packages in parallel should not lead to crashes.

As suggested by @bkhouri here, I ran WorkspaceTests multiple times to see if I run into any thread-safety issues:

for ((i=1; i<=30; i++)); do swift test --filter WorkspaceTests.WorkspaceTests; done
for ((i=1; i<=10; i++)); do swift test --filter WorkspaceTests; done

fortmarek · 2025-01-15T09:47:07Z

Sources/Workspace/Workspace+Dependencies.swift

-                    )
-                default:
-                    throw InternalError("invalid resolved package type \(resolvedPackage.packageRef.kind)")
+        await withThrowingTaskGroup(of: Void.self) { taskGroup in


Returning back the parallelization of retrieving resolved packages

fortmarek · 2025-01-15T09:47:51Z

Sources/Workspace/Workspace+State.swift


 /// Represents the workspace internal state persisted on disk.
-public final class WorkspaceState {
+public actor WorkspaceState {


Making WorkspaceState into actor to make it thread-safe

fortmarek · 2025-01-15T09:50:23Z

Sources/Workspace/ManagedDependency.swift

 extension Workspace {
    /// A collection of managed dependencies.
-    final public class ManagedDependencies {
+    public struct ManagedDependencies {


Turning ManagedDependencies into a struct. The alternative would be to make it an actor but since it conforms to Collection, it wouldn't be that easy.

There are only two methods of ManagedDependencies that are mutable – add and remove. Instead of mutating self, we can return copy of Self and do the mutation in WorkspaceState.

fortmarek · 2025-01-15T09:52:31Z

Sources/Workspace/Workspace+State.swift

+    public func add(dependency: Workspace.ManagedDependency) {
+        dependencies = dependencies.add(dependency)
+    }
+
+    public func remove(identity: PackageIdentity) {
+        dependencies = dependencies.remove(identity)
+    }


To keep the setter of dependencies private, we need to add helper methods for the two operations that mutate ManagedDependencies. Alternatively, we could make the setter public, however, I do find this makes callsites of WorkspaceState consumers nicer, so I'm leaning to keep the current solution.

bkhouri · 2025-01-15T21:16:01Z

I ran, from the root of the repository, the following shell script against this PR and there were no entries in the associated failed directory

#!/bin/bash
# cmd="swift test --very-verbose --filter CommandsTests.RunCommandTests/testMultipleExecutableAndExplicitExecutable"
cmd=(
    "git log -n1"
    "time swift test --filter \"Workspace*Tests\" --parallel --disable-swift-testing"
)
BASEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"


custom_run=$(date -u +'%Y%m%dT%H%M%S%Z')
root_log_dir="${HOME}/Downloads/can_be_deleted/swiftpm_logs/${custom_run}"
failed_dir="${root_log_dir}/failed"

caffeinate_cmd=$(command -v caffeinate)

function cleanup() {
    echo ""
    echo ""
    echo "******************************************************"
    echo "Root log directory  : ${root_log_dir}"
    echo "Failed log directory: ${failed_dir}"
}

trap cleanup EXIT

mkdir -p ${failed_dir}


num_iterations=300
# for num in $(seq -w 2); 
echo "Executing         : ${cmd}"
echo "Root Log Directory: ${root_log_dir}"
set -x 
which swift 
xcode-select -p
swift --version
set +x
for num in $(seq -w $num_iterations); 
do 
    (
        cd ${BASEDIR}
        log_dir=${root_log_dir}/${num}
        message="[${num}/${num_iterations}] executing and writing log to ${log_dir}"
        echo "${message} ..."
        mkdir -p ${log_dir}
        test_log=${log_dir}/swift_test_console_${num}.txt
        rm -rf ${test_log}
        start_time=$(date +%s)
        for ((i = 0; i < ${#cmd[@]}; i++))
        do
            full_cmd="${caffeinate_cmd} ${cmd[${i}]}"
            echo "❯❯❯ Executing: ${full_cmd}" >> ${test_log}
            # set -x
            # caffeinate ${cmd} 2>&1 | tee -a "${test_log}"
            ${full_cmd} >> "${test_log}" 2>&1
            rc=$?
            # set +x
            if [ ${rc} -ne 0 ] ; then
            # if [ ${PIPESTATUS[0]} -ne 0 ] ; then
                ln -s ${log_dir} ${failed_dir}
            fi

        done
        end_time=$(date +%s)
        echo "${message} completed in $(( ($end_time - $start_time) )) seconds"
        
        # echo "Waiting 0.5 second..."
        # echo "Done run number ${num}.  Log dir: ${log_dir}"
        sleep 0.5 
    )
done
echo "Done all ${um_iterations} interations"

fortmarek · 2025-01-15T22:54:47Z

Thanks for checking @bkhouri. I believe then the previous thread-safety issues should be fixed now 🙏 cc @dschaefer2

bkhouri · 2025-01-16T14:32:06Z

I added the utility script in #8227

fortmarek · 2025-01-16T21:20:02Z

Also, we did some performance testing of this branch at https://github.com/tuist/registry-tests using some real-worldPackage.swift files sent by folks from the Tuist community and clean checkouts (both when using registry or source control resolution) are between faster by 33 % to 51 %. In other words, clean checkouts will be up to 2x as fast once this ships.

Here's some of the relevant data we gathered:

Measurement data

Package 1 (41 dependencies)

Source control with Swift 6.0.3

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643543479#step:12:1276

Time (mean ± σ):     73.026 s ±  8.001 s    [User: 89.784 s, System: 17.401 s]
Range (min … max):   66.706 s … 86.556 s    5 runs

Registry with Swift 6.0.3

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643544361#step:6:760

Time (mean ± σ):     81.317 s ±  6.360 s    [User: 68.083 s, System: 10.738 s]
Range (min … max):   72.128 s … 86.903 s    5 runs

Source control using swift-package from this branch

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643543098#step:13:1006

Time (mean ± σ):     36.802 s ±  1.074 s    [User: 61.925 s, System: 9.012 s]
Range (min … max):   35.342 s … 37.982 s    5 runs

~51 % faster than the Swift 6.0.1 release

Registry using swift-package from this branch

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643544096#step:11:514

Time (mean ± σ):     42.102 s ±  3.453 s    [User: 68.086 s, System: 10.986 s]
Range (min … max):   38.757 s … 46.024 s    5 runs

~48 % faster than the Swift 6.0.1

Package 2 (63 dependencies)

Source control with Swift 6.0.3

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643545273#step:12:3070

Time (mean ± σ):     309.176 s ± 36.704 s    [User: 346.258 s, System: 67.350 s]
Range (min … max):   273.085 s … 348.343 s    5 runs

Registry with Swift 6.0.3

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643546221#step:6:2116

Time (mean ± σ):     289.427 s ± 18.107 s    [User: 173.334 s, System: 31.886 s]
Range (min … max):   263.536 s … 311.480 s    5 runs

Source control using swift-package from this branch

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643544647#step:13:2614

Time (mean ± σ):     196.464 s ± 13.283 s    [User: 219.689 s, System: 36.472 s]
Range (min … max):   185.316 s … 215.061 s    5 runs

~38 % faster than current swift release

Registry using swift-package from this branch

Source: https://github.com/tuist/registry-tests/actions/runs/12785286884/job/35643545584#step:11:1750

Time (mean ± σ):     193.458 s ±  5.784 s    [User: 122.490 s, System: 17.624 s]
Range (min … max):   185.861 s … 201.466 s    5 runs

~33 % faster than the current

bkhouri · 2025-01-17T02:20:19Z

@fortmarek Have you tried executing all of the swift tests on your change? Maybe it's me, but I'm seeing a few test failures when running swift test --parallel.

fortmarek · 2025-01-17T16:53:15Z

Thanks @bkhouri! I don't think it has to do anything with running the tests in parallel, but some PackageCommandTests are actually failing. I will look into it 🙂

fortmarek · 2025-01-18T21:35:03Z

@bkhouri the issue should be fixed – SwiftCommand should now conform to AsyncParsableCommand instead of ParsableCommand as the run method has been changed to being async.

bkhouri · 2025-01-23T16:08:04Z

@swift-ci please test macos linux

fortmarek · 2025-02-16T11:26:49Z

Hey @bkhouri, sorry the longer silence here.

Would you mind re-running the CI – I updated the branch with the latest changes from main and ran things locally with all the tests passing.

plemarquand · 2025-02-16T12:43:04Z

@swift-ci please test

fortmarek · 2025-02-17T12:10:56Z

Looks like all CI tests are passing now, too. Let me know if there's anything else I can do to push this PR forward 🙂

fortmarek · 2025-02-26T12:36:27Z

Hey @bkhouri @plemarquand @MaxDesiatov – sorry for the broad ping, but would love to see this get in 🙏

MaxDesiatov · 2025-02-26T12:37:28Z

@swift-ci test

pepicrft · 2025-02-26T17:24:16Z

This seems to be passing on CI 🚀

Sources/CoreCommands/SwiftCommandState.swift

Sources/Commands/PackageCommands/ResetCommands.swift

MaxDesiatov

With this PR as is, the relationship between SwiftCommand and AsyncSwiftCommand becomes unclear, they essentially duplicate each other with this change.

…and async

fortmarek · 2025-02-27T21:35:37Z

Thanks @MaxDesiatov for your feedback, should be resolved now 🙏

MaxDesiatov

LGTM, but I'd like this to get at least one review from someone on the SwiftPM team before merging.

MaxDesiatov · 2025-02-27T21:42:15Z

@swift-ci test

MaxDesiatov · 2025-02-27T21:42:25Z

@swift-ci test windows

MaxDesiatov · 2025-02-27T21:56:11Z

@swift-ci test windows

MaxDesiatov · 2025-02-27T23:37:01Z

@swift-ci test windows

plemarquand

Looks great to me! Just one minor comment, otherwise its good to go.

Sources/CoreCommands/BuildSystemSupport.swift

MaxDesiatov · 2025-02-28T15:10:13Z

@swift-ci test

MaxDesiatov · 2025-02-28T15:10:29Z

@swift-ci test windows

MaxDesiatov · 2025-02-28T15:21:34Z

@swift-ci test windows

plemarquand · 2025-03-03T21:01:04Z

@fortmarek thanks for this great contribution! Looking forward to the improvements it brings.

Make WorkspaceState into actor to make it thread-safe

e507854

fortmarek mentioned this pull request Jan 14, 2025

Revert "Retrieve resolved package versions in parallel" #8217

Merged

fortmarek added 2 commits January 15, 2025 09:34

Merge branch 'main' into workspace-state-actor

f326ad8

Parallelize retrieving resolved packages

0cf2d7c

fortmarek changed the title ~~Make WorkspaceState into actor to make it thread-safe~~ Paralellize retrieving resolved packages Jan 15, 2025

fortmarek commented Jan 15, 2025

View reviewed changes

Make mutating ManagedDependencies thread-safe

3d99328

fortmarek commented Jan 15, 2025

View reviewed changes

fortmarek marked this pull request as ready for review January 15, 2025 09:54

fortmarek requested review from MaxDesiatov, bkhouri, bnbarham, bripeticca, dschaefer2, francescomikulis, jakepetroules, owenv, plemarquand, shawnhyam and xedin as code owners January 15, 2025 09:54

Change SwiftCommand conformance to AsyncParsableCommand

7bdd76e

fortmarek added 2 commits February 16, 2025 11:56

Merge branch 'main' into workspace-state-actor

798bdc3

Add missing await

e996177

MaxDesiatov reviewed Feb 26, 2025

View reviewed changes

Sources/CoreCommands/SwiftCommandState.swift Outdated Show resolved Hide resolved

MaxDesiatov reviewed Feb 26, 2025

View reviewed changes

Sources/Commands/PackageCommands/ResetCommands.swift Show resolved Hide resolved

MaxDesiatov requested changes Feb 26, 2025

View reviewed changes

fortmarek added 3 commits February 27, 2025 22:29

Conform ResetCommand to AsyncSwiftCommand instead of making SwiftComm…

7f7c672

…and async

Conform SwiftCommand to ParsableCommand instead of AsyncParsableCommand

60f8942

Remove redundant async

5b1c7e2

fortmarek requested a review from MaxDesiatov February 27, 2025 21:35

MaxDesiatov approved these changes Feb 27, 2025

View reviewed changes

plemarquand approved these changes Feb 28, 2025

View reviewed changes

Sources/CoreCommands/BuildSystemSupport.swift Outdated Show resolved Hide resolved

Trigger async canUseCachedBuildManifest only when needed

3cda975

plemarquand approved these changes Feb 28, 2025

View reviewed changes

plemarquand merged commit 4e029ce into swiftlang:main Mar 3, 2025
5 checks passed

Paralellize retrieving resolved packages #8220

Paralellize retrieving resolved packages #8220

Uh oh!

Conversation

fortmarek commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

fortmarek Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

fortmarek Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

fortmarek Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

fortmarek Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

bkhouri commented Jan 15, 2025

Uh oh!

fortmarek commented Jan 15, 2025

Uh oh!

bkhouri commented Jan 16, 2025

Uh oh!

fortmarek commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Package 1 (41 dependencies)

Source control with Swift 6.0.3

Registry with Swift 6.0.3

Source control using swift-package from this branch

Registry using swift-package from this branch

Package 2 (63 dependencies)

Source control with Swift 6.0.3

Registry with Swift 6.0.3

Source control using swift-package from this branch

Registry using swift-package from this branch

Uh oh!

bkhouri commented Jan 17, 2025

Uh oh!

fortmarek commented Jan 17, 2025

Uh oh!

fortmarek commented Jan 18, 2025

Uh oh!

bkhouri commented Jan 23, 2025

Uh oh!

fortmarek commented Feb 16, 2025

Uh oh!

plemarquand commented Feb 16, 2025

Uh oh!

fortmarek commented Feb 17, 2025

Uh oh!

fortmarek commented Feb 26, 2025

Uh oh!

MaxDesiatov commented Feb 26, 2025

Uh oh!

pepicrft commented Feb 26, 2025

Uh oh!

Uh oh!

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

fortmarek commented Feb 27, 2025

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov commented Feb 27, 2025

Uh oh!

MaxDesiatov commented Feb 27, 2025

Uh oh!

MaxDesiatov commented Feb 27, 2025

Uh oh!

MaxDesiatov commented Feb 27, 2025

Uh oh!

plemarquand left a comment

fortmarek commented Jan 14, 2025 •

edited

Loading

fortmarek commented Jan 16, 2025 •

edited

Loading