Use Unicode.Scalar initializer for most character definitions #51
Conversation
|
Unfortunately the diff is not going to be particularly friendly if you want to guarantee correctness for literally every entry. There are tests, and they are good tests, but I do not believe they would catch every single mistake. |
|
@ianpartridge any thoughts on this? |
|
Hi @eskerber , can you please sign the CLA? |
|
@pushkarnk I signed it a while ago. The bot is not working. |
|
@eskerber Thanks for submitting this pull request! I can see that this change is working around an apparent Swift compiler issue and giving us a considerable performance boost. I am just curious to know if the compiler limitation is around "managing constants" or "handling constants in a dictionary". If the limitation is about constants in a dictionary, is it worth writing an embarrassingly long switch statement to map the Strings to Characters? If it is about managing constants overall, I don't think we can do much. Eager to know your views! |
|
@pushkarnk I posted the SR I created for this in the referenced issue: https://bugs.swift.org/browse/SR-10209 It is a performance regression in Swift 5 optimizations that the team is investigating. I imagine they will fix this in relatively short order (even though that means waiting for a new Swift/Xcode release), so I wouldn't necessarily spend too much time gutting the current implementation. It's difficult to know exactly if whatever inlining is being held up is simply a result of how many literals there are vs them being in a dictionary. For what it's worth, I did try to break up the dictionaries into a series of dictionary merges 4-5 key/value pairs at a time and it was still slow. |
|
There's no harm in accepting this PR as a work-around for a serious performance problem with the swift compiler. We must revisit this problem once there is a resolution for SR-10209. |
|
@eskerber The branch has conflicts. If you could rebase it, I'll go ahead and merge this. |
|
Hi @eskerber , just wanted to check if you could rebase your branch against the latest master. We can get this merged. Thanks! |
erikkerber
force-pushed
the
swift-4.2-optimization-fix
branch
from
abbc82b to
a9003b6
Compare
|
Sorry - usually spend my time in enterprise Github Rebased 👍 |
|
Oh, the failures seem to be Travis issues. We're seeing them on other repos too. |
|
Yes @djones6 and I know what the problem is. Will fix ASAP. |
|
Hi @djones6 , @eskerber has signed the CLA but the bot doesn't seem to report so. Can you please advise what we could do here? |
|
Can we test whether the slow compilation is still a problem in Swift 5.0.1? I know there were some optimizer performance fixes included in 5.0.1 so if the problem in the compiler is fixed that would make this PR moot? |
|
I believe it was still an issue when we checked. |
|
OK thanks for checking. Let's release this 👍 |
Description
To resolve a drastic degradation in compiling optimized Swift code using, as is present in Constants.swift, thousands of literals, a large conversion to using Character initializers helped the build process at least complete on hardware that is-of-this-world.
Important to note that any character that required multiple code points, I kept the original escaped String sequence.
Motivation and Context
Issue as described in #50
How Has This Been Tested?
Unit tests pass, archive completes.
Checklist: