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
[SR-6947] DoubleWidth compile time. GenericSignatureBuilder::enumeratePairedRequirements. #49495
Comments
There are actually multiple compile-time problems here, and potentially an issue with exposing a stdlib type like this in the first place. @DougGregor, I have a radar for the GenericSignatureBuilder here. Obviously we either need to cache something or a faster implementation.
@eeckstein has a separate radar for the code size problem. astmus (JIRA User) is following the introduction of this type in the stdlib. I don't think it should be exposed to users until we know that the compiler problems can be fixed. |
Minor historical note: the pulled DoubleWidth out of Swift 4.1 due to code-size problems, so despite having been in the tree for a long time, we haven't shipped it. |
Should that be @moiseev? |
@benrimmington Yes, thanks. I autocompleted "mois". |
/cc @natecook1000 and @xwu |
🙁 I apologize for blowing up CI. The benchmark has served its initial purpose in showing that revisions to the division algorithm produce a 700% boost in optimized runtime performance. Now, I guess, it's a test case for compiler performance. Without particular experience in the area, I have to wonder if this is only the first example (because we have a standard library type designed that way) of potentially pathological behavior arising from conditional conformance and recursive generic parameters in the context of complex protocol hierarchies–i.e., something that can arise in user code and not something truly unique to `DoubleWidth`. In the meantime, one thought is whether, at least with respect to compile-time problems, we could ship a slightly limited `DoubleWidth` that supports only non-`DoubleWidth` base integer types (i.e., `DoubleWidth<Base> where Base : _BasicFixedWidthInteger`) instead of keeping the type hidden for yet another release. |
There's no reason to think the compiler problems are specific to DoubleWidth. It's just that the stdlib should not vend a type that we know isn't going to be well supported in the compiler. And I want that support to be a complete long-term rethink of the design, not a few partial short-term workarounds. If we can ship the same functionality without recursive generic parameters, that would be great. We would be much better positioned in the future if it turns out that we can't compile the recursive generic case as efficiently. |
It looks like this performance issue was fixed a while ago; I added Andy's code to test/Prototypes/DoubleWidth.swift.gyb and built it with -O -Xllvm -sil-partial-specialization -Xllvm -sil-inline-generics. We spend very little time in the GSB or the requirement machine, respectively, when the requirement machine is off and on. |
Additional Detail from JIRA
md5: 205feff3be612eca85bed9ad56477f1e
Issue Description:
Adding the DoubleWidth benchmark broke the benchmark suite for practical purposes.
It should take under 4 minutes to build the benchmarks. Now it takes closer to 20 minutes.
This five-fold increase is actually much worse on machines with more
cores, so this is also our draining our CI. I'll need to disable the benchmark for now.
You can reproduce the compile time bug with this tiny 20 line program that takes over 15 minutes to compile:
Almost all the time appears to be spent in
`swift::GenericSignature::enumeratePairedRequirements(llvm::function_ref<bool (swift::Type, llvm::ArrayRef<swift::Requirement>)>)` const
Note that there is also an an extreme amount of specialization happenening, but that's the intention of the DoubleWidth API. Disabling specialization would actually break the API's contract. If the specialization is a problem, then we really have a stdlib bug and
DoubleWidth should be not be shipped as-is.
The text was updated successfully, but these errors were encountered: