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
GSB: New algorithm for computing redundant requirements #37154
GSB: New algorithm for computing redundant requirements #37154
Conversation
@swift-ci Please test source compatibility |
@swift-ci Please smoke test |
@swift-ci Please test macOS |
bb85c00
to
df0a43d
Compare
40c6cfb
to
434600b
Compare
@swift-ci Please test |
@swift-ci Please test source compatibility |
@swift-ci Please test compiler performance |
@swift-ci Please test source compatibility debug |
@swift-ci Please test Windows |
Unfortunately the perf suite results are bogus because the number of source lines doesn't match between the before and after jobs. |
434600b
to
c302986
Compare
@swift-ci Please test |
@swift-ci Please test source compatibility |
@swift-ci Please smoke test |
@swift-ci Please test source compatibility debug |
This rewrites the existing redundant requirements algorithm to be simpler, and fix an incorrect behavior in the case where we're building a protocol requirement signature. Consider the following example: protocol P { associatedtype A : P associatedtype B : P where A.B == B } The requirement B : P has two conformance paths here: (B : P) (A : P)(B : P) The naive redundancy algorithm would conclude that (B : P) is redundant because it can be derived as (A : P)(B : P). However, if we drop (B : P), we lose the derived conformance path as well, since it involves the same requirement (B : P). The above example actually worked before this change, because we handled this case in getMinimalConformanceSource() by dropping any derived conformance paths that involve the requirement itself appearing in the "middle" of the path. However, this is insufficient because you can have a "cycle" here with length more than 1. For example, protocol P { associatedtype A : P where A == B.C associatedtype B : P where B == A.C associatedtype C : P where C == A.B } The requirement A : P has two conformance paths here: (A : P) (B : P)(C : P) Similarly, B : P has these two paths: (B : P) (A : P)(C : P) And C : P has these two paths: (C : P) (A : P)(B : P) Since each one of A : P, B : P and C : P has a derived conformance path that does not involve itself, we would conclude that all three were redundant. But this was wrong; while (B : P)(C : P) is a valid derived path for A : P that allows us to drop A : P, once we commit to dropping A : P, we can no longer use the other derived paths (A : P)(C : P) for B : P, and (A : P)(B : P) for C : P, respectively, because they involve A : P, which we dropped. The problem is that we were losing information here. The explicit requirement A : P can be derived as (B : P)(C : P), but we would just say that it was implied by B : P alone. For non-protocol generic signatures, just looking at the root is still sufficient. However, when building a requirement signature of a self-recursive protocol, instead of looking at the root explicit requirement only, we need to look at _all_ intermediate steps in the path that involve the same protocol. This is implemented in a new getBaseRequirements() method, which generalizes the operation of getting the explicit requirement at the root of a derived conformance path by returning a vector of one or more explicit requirements that appear in the path. Also the new algorithm computes redundancy online instead of building a directed graph and then computing SCCs. This is possible by recording newly-discovered redundant requirements immediately, and then using the set of so-far-redundant requirements when evaluating a path. This commit introduces a small regression in an existing test case involving a protocol with a 'derived via concrete' requirement. Subsequent commits in this PR fix the regression and remove the 'derived via concrete' mechanism, since it is no longer necessary. Fixes https://bugs.swift.org/browse/SR-14510 / rdar://problem/76883924.
…nce requirements This fixes a regression from the new redundant requirements algorithm and paves the way for removing the notion of 'derived via concrete' requirements.
…ivalence classes Literally an off-by-one error -- we were skipping over the first requirement and not copying it over. I think this is because the updateLayout() call used to be addLayoutRequirementDirect(), which would add the first layout constraint, and I forgot to remove the '+ 1' when I refactored this.
…undedness Consider this example: protocol P1 { associatedtype A : P2 } protocol P2 { associatedtype A } func foo<T : P2>(_: T) where T.A : P1, T.A.A == T {} We cannot drop 'T : P2', even though it can be derived from T.A.A == T and Self.A : P2 in protocol P1, because we actually need the conformance T : P2 in order to recover the concrete type for T.A. This was handled via a hack which would ensure that the conformance path (T.A : P1)(Self.A : P2) was not a candidate derivation for T : P2, because T : P2 is one of the conformances used to construct the subject type T.A in the conformance path. This hack did not generalize to "cycles" of length greater than 1 however: func foo<T : P2, U : P2>(_: T, _: U) where T.A : P1, T.A.A == U, U.A : P1, U.A.A == T {} We used to drop both T : P2 and U : P2, because each one had a conformance path (U.A : P1)(Self.A : P2) and (T.A : P1)(Self.A : P2), respectively; however, once we drop T : P2 and U : P2, these two paths become invalid for the same reason that the concrete type for T.A and U.A can no longer be recovered. The correct fix is to recursively visit the subject type of each base requirement when evaluating a conformance path, and not consider any requirement whose subject type's parent type cannot be recovered. This proceeds recursively. In the worst case, this algorithm is exponential in the number of conformance requirements, but it is not always exponential, and a generic signature is not going to have a large number of conformance requirements anyway (hopefully). Also, the old logic in getMinimalConformanceSource() wasn't cheap either. Perhaps some clever minimization can speed this up too, but I'm going to focus on correctness first, before looking at performance here. Fixes rdar://problem/74890907.
This used to be necessary to handle this case: protocol P1 { associatedtype A } protocol P2 { associatedtype A : P1 } func foo<T : P1>(_: T) where T.A : P2, T.A.A == T {} We don't want to mark 'T : P1' as redundant, even though it could be recovered from T.A.A, because it uses the requirement T.A, and we cannot recover the type metadata for T.A without the witness table for T : P1. Now this is handled via a more general mechanism, so we no longer need this hack.
Consider a signature with a conformance requirement, and two identical superclass requirements, both of which make the conformance requirement redundant. The conformance will have two requirement sources, the explicit source and a derived source based on one of the two superclass requirements, whichever one was processed first. If we end up marking the *other* superclass requirement as redundant, we would incorrectly conclude that the conformance was not redundant. Instead, if a derived source is based on a redundant requirement, we can't just discard it right away; instead, we have to check if that source could have been derived some other way.
c302986
to
e45b566
Compare
@swift-ci Please smoke test |
@swift-ci Please test source compatibility |
@swift-ci Please test compiler performance |
@swift-ci Please test Linux |
Summary for main fullRegressions found (see below) Debug-batchdebug-batch briefRegressed (2)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (0)
debug-batch detailedRegressed (215)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (35)
Releaserelease briefRegressed (0)
Improved (2)
Unchanged (delta < 1.0% or delta < 100.0ms) (0)
release detailedRegressed (0)
Improved (208)
Unchanged (delta < 1.0% or delta < 100.0ms) (42)
|
@slavapestov seems that this change has an issue on Windows. Could you please look into it or revert it until that can be fixed? |
This PR now contains changes from two PRs.
First PR
This rewrites the existing redundant requirements algorithm to be simpler, and fix an incorrect behavior in the case where
we're building a protocol requirement signature.
Consider the following example:
The requirement B : P has two conformance paths here:
The naive redundancy algorithm would conclude that (B : P) is redundant because it can be derived as (A : P)(B : P). However, if we drop (B : P), we lose the derived conformance path as well, since it involves the same requirement (B : P).
The above example actually worked before this change, because we handled this case in getMinimalConformanceSource() by dropping any derived conformance paths that involve the requirement itself appearing in the "middle" of the path.
However, this is insufficient because you can have a "cycle" here with length more than 1. For example,
The requirement A : P has two conformance paths here:
Similarly, B : P has these two paths:
And C : P has these two paths:
Since each one of A : P, B : P and C : P has a derived conformance path that does not involve itself, we would conclude that all three were redundant. But this was wrong; while (B : P)(C : P) is a valid derived path for A : P that allows us to drop A : P, once we commit to dropping A : P, we can no longer use the other derived paths (A : P)(C : P) for B : P, and (A : P)(B : P) for C : P, respectively, because they involve A : P, which we dropped.
The problem is that we were losing information here. The explicit requirement A : P can be derived as (B : P)(C : P), but we would just say that it was implied by B : P alone.
For non-protocol generic signatures, just looking at the root is still sufficient.
However, when building a requirement signature of a self-recursive protocol, instead of looking at the root explicit requirement only, we need to look at all intermediate steps in the path that involve the same protocol.
This is implemented in a new getBaseRequirements() method, which generalizes the operation of getting the explicit requirement at the root of a derived conformance path by returning a vector of one or more explicit requirements that appear in the path.
Also the new algorithm computes redundancy online instead of building a directed graph and then computing SCCs. This is possible by recording newly-discovered redundant requirements immediately, and then using the set of so-far-redundant requirements when evaluating a path.
Fixes https://bugs.swift.org/browse/SR-14510 / rdar://problem/76883924.
Second PR
Consider this example:
We cannot drop 'T : P2', even though it can be derived from T.A.A == T and Self.A : P2 in protocol P1, because we actually
need the conformance T : P2 in order to recover the concrete type for T.A.
This was handled via a hack which would ensure that the conformance path (T.A : P1)(Self.A : P2) was not a candidate
derivation for T : P2, because T : P2 is one of the conformances used to construct the subject type T.A in the conformance path.
This hack did not generalize to "cycles" of length greater than 1 however:
We used to drop both T : P2 and U : P2, because each one had a conformance path (U.A : P1)(Self.A : P2) and (T.A : P1)(Self.A : P2), respectively; however, once we drop T : P2 and U : P2, these two paths become invalid for the same reason that the concrete type for T.A and U.A can no longer be recovered.
The correct fix is to recursively visit the subject type of each base requirement when evaluating a conformance path, and not consider any requirement whose subject type's parent type cannot be recovered.
In the worst case, this algorithm is exponential in the number of conformance requirements, but it is not always exponential, and a generic signature is not going to have a large number of conformance requirements anyway (hopefully). Also, the old logic in getMinimalConformanceSource() wasn't cheap either.
Perhaps some clever memoization can speed this up too, but I'm going to focus on correctness first, before looking at performance here.
Fixes rdar://problem/74890907.