Revert "Retrieve resolved package versions in parallel"

[dschaefer2]

Reverts #8203

This is causing crashes in SwiftPM during the parallelized checkout. The checkouts update the workspace state and then save it. The workspace save data is not protected against multi-thread access while the save is walking the data which is causing the crash.

[dschaefer2]

@swift-ci please test

[fortmarek]

@dschaefer2 what do you think about parallelizing only downloads of registry packages? That is, instead of parallelizing both checkoutRepository and downloadRegistryArchive, we'd parallelize only the latter? Alternatively, checkoutRepository could be made thread-safe, but I'm not sure how complex that would be.

The parallelization makes a really big difference in performance, so still would love to make it work.

[fortmarek]

This is what I had in mind: #8218

[dschaefer2]

I think the right solution is to make Workspace an actor so that its state is concurrency safe. That's one thing I've noticed working on SwiftPM the last few months is how so much of it predates Swift concurrency and really hasn't adopted it fully. However that's also a pretty big change since all clients would need to become async as well.

I was actually excited about this change for the non-registry case since that's a common complaint I've seen as well. But looking at the registry case, it is also changing the list of dependencies in the Workspace so would have the same problem anyway.

[fortmarek]

I think the right solution is to make Workspace an actor so that its state is concurrency safe. That's one thing I've noticed working on SwiftPM the last few months is how so much of it predates Swift concurrency and really hasn't adopted it fully. However that's also a pretty big change since all clients would need to become async as well.

Is this something the SwiftPM team plans to work on? Any way we can push it forward? The sequential download of registry is quite a drag on its performance – and so is for source control checkouts. I would love to push this initiative further, but there's also so much time I can dedicate to this and it sounds like making Workspace an actor could have a little of ripple effects.

But looking at the registry case, it is also changing the list of dependencies in the Workspace so would have the same problem anyway.

Got it. Let's go with the revert. I do wonder in the interim, if we could at least split the download operation and updating the state into two steps. However, it's definitely preferred to make Workspace an actor, I'm just uncertain how complex that would be.

[dschaefer2]

Is this something the SwiftPM team plans to work on?

We're all the SwiftPM team, it's open source :). But the people I work with are allocated to other things for the next few months so nothing would be imminent from us.

If you can think of a compromise that makes sure this parallelization serializes access to the Workspace state I would be OK with that for a short term solution.

Though looking at the code, it might be sufficient to make WorkspaceState an actor which has a much smaller API service and the affects might not be so bad. Looking at the updateCheckout for example, it's already async.

[fortmarek]

If you can think of a compromise that makes sure this parallelization serializes access to the Workspace state I would be OK with that for a short term solution. Though looking at the code, it might be sufficient to make WorkspaceState an actor which has a much smaller API service and the affects might not be so bad. Looking at the updateCheckout for example, it's already async.

I'll try having a look at this. Thanks for the pointers and sorry for breaking things!

[dschaefer2]

I'll try having a look at this. Thanks for the pointers and sorry for breaking things!

All good. It helped highlight we really need to make SwiftPM more thread safe. As a central piece of the Swift toolchain, we really should be able to hold it up as a good example of modern Swift. :).

[fortmarek]

@dschaefer2 here's a PR that makes WorkspaceState into an actor, it wasn't too bad: #8220

Were you able to consistently reproduce the crash? I ran the test suite multiple times and things look a-ok, but would love to double check.

[bkhouri]

@swift-ci please test windows

[bkhouri]

@dschaefer2 here's a PR that makes WorkspaceState into an actor, it wasn't too bad: #8220

Were you able to consistently reproduce the crash? I ran the test suite multiple times and things look a-ok, but would love to double check.

@fortmarek personally, the WorkspaceTests failed for me 7 out of 30. I can try the same with #8220 tomorrow!