[stdlib] Constrain BinaryInteger.Words to be a Collection #11178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can it be a stronger requirement, like a |
|
@swift-ci Please smoke test |
|
@moiseev Not yet (or at least not as easily): it leads to seemingly unrelated errors in BidirectionalCollection.swift and elsewhere, and then a compiler crash. I'll see if I can make sense of it. It is also not yet possible to have |
|
@lorentey I don't understand your second point about inability to use |
|
@moiseev Me neither! Something gets confused when (It goes on for a couple dozen more errors like this.) Does this ring a bell for you? |
This begs for a another equality constraint like |
|
@swift-ci Please smoke benchmark |
|
Since messing with constraints like this sometimes has an unwelcome changes in compilation time, I'd rather not merge this change without measuring the stdlib compile time with and without this patch applied. |
|
Understood! 👍 Shall I time it on my end? FWIW, my timings for incremental builds using |
|
/cc @airspeedswift see above on the compilation time numbers. |
Build comment file:Optimized (O)Improvement (2)
No Changes (325)
Unoptimized (Onone)Improvement (1)
No Changes (326)
Hardware Overview |
This resolves a build time performance regression introduced in the previous commit.
This allows the use of these properties on BinaryInteger.Words in unconstrained generic constraints.
lorentey
force-pushed
the
words-collection
branch
from
2da77c9
to
6940054
Compare
|
@swift-ci Please smoke test |
|
I fixed the build time regression by relaxing the BinaryInteger.Words constraint to To ensure Updated build times: (incremental |
|
@swift-ci Please test |
|
FWIW, here are my local measurements of build time with and without the change applied: Regression of about ~10%. |
|
@moiseev Weird; I get consistently better ratios. (On an i7 rMBP with -j8, with the PR rebased onto current master.) With
With
In any case, instance-level |
|
@lorentey what are you timing exactly? I just cannot explain why I see the difference and you don't =) My procedure is essentially this: utils/build-script -r --skip-build-benchmarks
find ./stdlib/public/core/* -name '*.swift*' -print0 | xargs -0 touch
time ninja -C ../build/Ninja-RelWithDebInfoAssert+stdlib-DebugAssert/swift-macosx-x86_64 swift-stdlib-macosxWhat's yours? I tried it again just now on my desktop with these results: |
|
@moiseev Ah, that explains it! I just used |
|
This is now obsolete; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BinaryInteger.Wordscould previously only be constrained to aSequenceofUInts; it is now possible to constrain it toCollection, so that we match the interface accepted in SE-104.