Make `Resolved*` in `PackageGraph` value types #7160

MaxDesiatov · 2023-12-02T23:17:44Z

Depends on #7161. Unblocks macros cross-compilation fix in #7118.

Motivation:

Value types are easier to reason about as they prevent "spooky action at a distance" bugs, and can be easily made Equatable, Hashable, and most important of all Sendable.

Modifications:

Converted ResolvedTarget, ResolvedProduct, and ResolvedPackage from final classes to structs.

Renamed underlyingTarget, underlyingProduct, and underlyingPackage to just underlying to avoid tautological patterns like target.underlyingTarget.

Result:

It's easier to apply changes in PackageGraph. Also, in my benchmarks done locally this makes package graph resolution consistently ~5% faster when resolving the package graph of SwiftPM's own Package.swift.

`main` branch:

╒══════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric                           │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞══════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Time (system CPU) (ms)           │       7 │       8 │      11 │      14 │      15 │      18 │      18 │      25 │
├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ms)            │      12 │      13 │      16 │      19 │      20 │      23 │      23 │      25 │
├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (user CPU) (μs)             │    4688 │    4857 │    4890 │    4956 │    5107 │    5347 │    5347 │      25 │
├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (wall clock) (ms)           │     390 │     397 │     402 │     409 │     414 │     420 │     420 │      25 │
╘══════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

This PR's branch:

╒══════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric                           │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞══════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Time (system CPU) (ms)           │       6 │       8 │      10 │      13 │      16 │      17 │      17 │      26 │
├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ms)            │      11 │      13 │      15 │      18 │      20 │      21 │      21 │      26 │
├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (user CPU) (μs)             │    4476 │    4567 │    4648 │    4796 │    4931 │    4998 │    4998 │      26 │
├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (wall clock) (ms)           │     375 │     379 │     384 │     391 │     394 │     398 │     398 │      26 │
╘══════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

Future directions:

I'm unable to fully remove Resolved*Builder classes yet, as they rely on Target being a reference type and module aliasing relying on side effects of modifying instances of Target. In turn, Target is a class hierarchy, which we should convert to multiple value types that use composition in the future. In the meantime this also prevents us from making it, and all types that contain it, Sendable.

Multiple places in our codebase pass around and store `[any PackageConditionProtocol]` arrays of existentials with `FIXME` notes that those should be sets and not arrays. Adding `Hashable` requirement to `PackageConditionProtocol` doesn't fix the issue, since existentials of this protocol still wouldn't conform to `Hashable`. A simple alternative is to create `enum PackageCondition` with cases for each condition type. We only have two of those types and they're always declared in the same package. There's no use case for externally defined types for this protocol. That allows us to convert uses of `[any PackageConditionProtocol]` to `Set<PackageCondition>`. Existing protocol kept around until clients of SwiftPM can migrate to the new `PackageCondition` enum.

Depends on #7117, unblocks #7160. The `PackageModel` module has no business of defining supported platforms computation for resolved targets, products, and packages. This belongs to `PackageGraph`, but was previously leaking out into `PackageModel` by passing the `derivedXCTestPlatformProvider` closure around. That prevented us from converting marking `SupportedPlatforms` as `Hashable` and marking any of `Resolved*` types as value types and also `Hashable`. In the future, when we'll also need to make them `Sendable`, this could become problematic, passing so much state captured in a closure, mostly by accident.

Unblocks `PackageGraph` value types refactoring in #7160 and macros cross-compilation fix in #7118. Multiple places in our codebase pass around and store `[any PackageConditionProtocol]` arrays of existentials with `FIXME` notes that those should be sets and not arrays. We also can't convert types containing these arrays to value types with auto-generated `Hashable` conformance, since neither elements of these arrays nor arrays themselves are `Hashable`. Adding `Hashable` requirement to `PackageConditionProtocol` doesn't fix the issue, since existentials of this protocol still wouldn't conform to `Hashable`. A simple alternative is to create `enum PackageCondition` with cases for each condition type. We only have two of those types and they're always declared in the same module. There's no use case for externally defined types for this protocol. That allows us to convert uses of `[any PackageConditionProtocol]` to `[PackageCondition]`. Existing protocol is kept around until clients of SwiftPM can migrate to the new `PackageCondition` enum. This also allows us to remove a custom conformance to `Hashable` on `ResolvedTarget`, unblocking a conversion of `ResolvedTarget` to a value type and making it `Sendable` in the future.

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

Sources/Build/BuildDescription/ClangTargetBuildDescription.swift

tomerd · 2023-12-05T17:45:33Z

Sources/PackageGraph/PackageGraphRoot.swift

@@ -102,6 +102,7 @@ public struct PackageGraphRoot {
        // at which time the current special casing can be deprecated.
        var adjustedDependencies = input.dependencies
        if let explicitProduct {
+            // FIXME: `dependenciesRequired` modifies manifests and prevents conversion of `Manifest` to a value type


Sources/PackageLoading/PackageBuilder.swift

…ft-package-manager into maxd/packagegraph-value-types # Conflicts: # Sources/Build/BuildDescription/TargetBuildDescription.swift # Sources/Build/BuildPlan/BuildPlan+Clang.swift # Sources/Build/BuildPlan/BuildPlan+Product.swift # Sources/Build/BuildPlan/BuildPlan+Swift.swift # Sources/PackageGraph/Resolution/ResolvedProduct.swift # Sources/PackageGraph/Resolution/ResolvedTarget.swift

### Motivation: The `PackageModel` module has no business of defining supported platforms computation for resolved targets, products, and packages. This belongs to `PackageGraph`, but was previously leaking out into `PackageModel` by passing the `derivedXCTestPlatformProvider` closure around. That prevented us from marking `SupportedPlatforms` as `Hashable` and marking any of `Resolved*` types as value types also `Hashable`. In the future, when we need to make them `Sendable`, passing mutable state captured in a closure could become problematic. This also unblocks `PackageGraph` value types refactoring in #7160 and macros cross-compilation fix in #7118. ### Modifications: Add new `PlatformVersionProvider` type, which is a value type that can be easily made `Hashable` and `Sendable`. Instead of passing closures around, platform version computation code is now gathered in a single file (new `PlatformVersionProvider.swift`), with all possible context captured in its inner `enum Implementation`. This new type is adopted by `Resolved*` types in the `PackageGraph` module, which actively uses minimum platform version computations. Renamed `func getDerived` to `func getSupportedPlatform`, now that this function delegating to `PlatformVersionProvider` is declared on `Resolved*` types. ### Result: `SupportedPlatforms` type, which is nothing more than `[SupportedPlatform]` array with an additional closure, can be removed. Types that previously had properties of `SupportedPlatforms` can be easily converted to value types and made `Hashable` in a way that accounts for their content instead of class instance identity.

…xd/packagegraph-value-types # Conflicts: # Sources/PackageGraph/PackageGraph+Loading.swift # Sources/PackageGraph/Resolution/PlatformVersionProvider.swift # Sources/PackageGraph/Resolution/ResolvedPackage.swift # Sources/PackageGraph/Resolution/ResolvedProduct.swift # Sources/PackageGraph/Resolution/ResolvedTarget.swift # Sources/XCBuildSupport/PIFBuilder.swift # Tests/PackageGraphTests/TargetTests.swift

MaxDesiatov · 2023-12-13T20:54:36Z

@swift-ci test

MaxDesiatov · 2023-12-13T20:54:55Z

@swift-ci test windows

MaxDesiatov · 2023-12-14T16:55:28Z

@swift-ci test

MaxDesiatov · 2023-12-14T17:20:01Z

@swift-ci test

MaxDesiatov · 2023-12-14T17:31:35Z

@swift-ci test windows

MaxDesiatov · 2023-12-14T17:43:16Z

@swift-ci test

MaxDesiatov · 2023-12-14T17:43:34Z

@swift-ci test windows

MaxDesiatov · 2023-12-15T01:59:32Z

@swift-ci test windows

Fix fixes a serious performance regression introduced for deep target dependency graphs in #7160. After converting `ResolvedTarget` to a value type, its synthesized `Hashable` conformance traversed the whole dependency tree multiple times when using `topologicalSort`. This made package resolution seemingly hang for packages that had a lot of nested target graphs. We already have an implementation of `topologicalSort` on `Identifiable`, thus we should use that instead. I've also added a performance test for this in `PackageGraphPerfTests` to prevent future regressions in this area.

### Motivation: A serious performance regression was introduced for deep target dependency graphs in #7160. After converting `ResolvedTarget` to a value type, its synthesized `Hashable` conformance traversed the whole dependency tree multiple times when using `topologicalSort`. This made package resolution seemingly hang for packages that had a lot of nested target graphs. ### Modifications: We already have an implementation of `topologicalSort` on `Identifiable`, so we're using that now instead. I've also added a performance test for this in `PackageGraphPerfTests` to prevent future regressions in this area. ### Result: Resolved rdar://119807385.

MaxDesiatov added 9 commits December 2, 2023 20:38

Add doc comment to the new type

6fd1606

Fix tests regression

456959e

Remove custom ResolvedTarget.Dependency conformance

420f01a

Roll back to arrays and reduce the diff

76121f4

Update comments and reduce the diff

4bd4bb4

Reduce the diff

d6c0c8a

Reduce the diff

f3004a7

Reduce the diff to track the regression

a6760d9

MaxDesiatov added the modules graph Modules dependency resolution label Dec 2, 2023

MaxDesiatov self-assigned this Dec 2, 2023

This was referenced Dec 2, 2023

Support macros when cross-compiling #7118

Merged

[NFC] Clean up package graph caching code #7128

Closed

MaxDesiatov mentioned this pull request Dec 3, 2023

[NFC] Remove SupportedPlatforms, add PlatformVersionProvider #7161

Merged

MaxDesiatov added 3 commits December 3, 2023 00:09

Use value types for storage in Resolved* types of PackageGraph

1dee112

Make Resolved* in PackageGraph value types

086d0f9

Minor cleanups

27d4f68

MaxDesiatov force-pushed the maxd/packagegraph-value-types branch from 9ba50be to 27d4f68 Compare December 3, 2023 00:10

MaxDesiatov changed the base branch from maxd/package-condition to maxd/platform-version-provider December 3, 2023 00:12

MaxDesiatov marked this pull request as ready for review December 3, 2023 00:12

MaxDesiatov requested review from neonichu and tomerd as code owners December 3, 2023 00:12

MaxDesiatov mentioned this pull request Dec 3, 2023

[NFC] Replace PackageConditionProtocol with PackageCondition #7117

Merged

Fix build errors

b6c2698

MaxDesiatov added the performance Performance optimizations and improvements label Dec 3, 2023

MaxDesiatov added 2 commits December 5, 2023 17:15

Merge branch 'main' into maxd/platform-version-provider

9387af3

Rename getDerived to getSupportedPlatform

b777e12

tomerd approved these changes Dec 5, 2023

View reviewed changes

tomerd reviewed Dec 5, 2023

View reviewed changes

Sources/PackageLoading/PackageBuilder.swift Outdated Show resolved Hide resolved

MaxDesiatov added 3 commits December 6, 2023 10:09

Sort cases in PlatformRegistry, use init in PackageBuilder

477935a

Merge branch 'main' into maxd/platform-version-provider

5b6670e

neonichu approved these changes Dec 12, 2023

View reviewed changes

Base automatically changed from maxd/platform-version-provider to main December 13, 2023 20:13

MaxDesiatov added 3 commits December 14, 2023 12:26

clean up formatting

5ecf03d

Add dependency equality test in TargetTests

c6fcbea

Fix target dependencies equality

ad5792c

MaxDesiatov force-pushed the maxd/packagegraph-value-types branch from 693b4ae to ad5792c Compare December 14, 2023 16:55

Use topologicalSort on Identifiable, fix PackageGraphPerfTests

02d5dfb

MaxDesiatov enabled auto-merge (squash) December 14, 2023 17:20

MaxDesiatov disabled auto-merge December 14, 2023 17:21

Fix formatting

c3ab4f5

MaxDesiatov enabled auto-merge (squash) December 15, 2023 02:01

MaxDesiatov merged commit b368b96 into main Dec 15, 2023

MaxDesiatov deleted the maxd/packagegraph-value-types branch December 15, 2023 04:31

MaxDesiatov mentioned this pull request Dec 20, 2023

Use topologicalSort with Identifiable on ResolvedTarget #7211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Resolved*` in `PackageGraph` value types #7160

Make `Resolved*` in `PackageGraph` value types #7160

MaxDesiatov commented Dec 2, 2023 •

edited

Loading

tomerd Dec 5, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 15, 2023

Make Resolved* in PackageGraph value types #7160

Make Resolved* in PackageGraph value types #7160

Conversation

MaxDesiatov commented Dec 2, 2023 • edited Loading

Motivation:

Modifications:

Result:

main branch:

This PR's branch:

Future directions:

tomerd Dec 5, 2023

Choose a reason for hiding this comment

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 14, 2023

MaxDesiatov commented Dec 15, 2023

Make `Resolved*` in `PackageGraph` value types #7160

Make `Resolved*` in `PackageGraph` value types #7160

MaxDesiatov commented Dec 2, 2023 •

edited

Loading

`main` branch: