[swift-4.0] Fix SIL serialization of witness tables and protocol witness thunks #10384
Conversation
…und. - SILSerializeAll flag is now stored in the SILOptions and passed around as part of it - Explicit SILSerializeAll/wholeModuleSerialized/makeModuleFragile API parameters are removed in many places
…alization of SIL witness tables This option is supposed to be used only for compiling overlays. User code should never be compiled with this option.
Serialize witness tables only if it -sil-serialize-all or -sil-serialize-witness-tables options are provided, which should happen only when building the stdlib and overlays, or it is a compilation with explicitly enabled resilience support. Disable serialization of witness tables in all other cases. Disabling the serialization of witness tables also avoids serialization of protocol witness thunks, whose serialization in 3rd party code sometimes resulted in performance degradation, because resilience checking rules used by many parts of the compiler would prohibit certain optimizations like inlining or specialization, if the caller was serialized and the callee would not be serialized. This should fix rdar://32718853 and address rdar://32743391
|
@swift-ci please test |
|
LGTM! |
|
@swift-ci Please benchmark |
Build comment file:Build failed before running benchmark. |
|
@swift-ci Please benchmark |
Build comment file:Build failed before running benchmark. |
|
@swift-ci Please benchmark |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
Build comment file:Build failed before running benchmark. |
|
@swiftix We don't currently support swift-4.0-branch benchmark testing, its only supported on master branch at this time. I will look into fixing this issue soon. Thanks! |
|
I'll run the benchmarks locally |
|
What's the status of this? (I'm waiting on it for #10482.) |
|
I'm informed that @swiftix is on vacation for a while, is someone else on @bob-wilson's team going to take over getting this in? (I'm not literally blocked, but the master equivalent of #10482 was rebased in a moderately non-trivial way over the master equivalent of this, so that PR is set up assuming the same will happen.) |
|
I think @eeckstein was following up on this PR. |
|
Yes, I still want to run benchmarks (locally). |
|
I did run the benchmarks locally and saw this regressions: Regression (12): StringHasPrefix 15 30 +100.0% 0.50x I'd like to investigate this before I merge @huonw Does this block you? |
|
Ok, I turns out that those regressions are not real. |
Serialize witness tables only if it -sil-serialize-all or -sil-serialize-witness-tables options are provided, which should happen only when building the stdlib and overlays, or it is a compilation with explicitly enabled resilience support. Disable serialization of witness tables in all other cases.
Disabling the serialization of witness tables also avoids serialization of protocol witness thunks, whose serialization in 3rd party code sometimes resulted in performance degradation, because resilience checking rules used by many parts of the compiler would prohibit certain optimizations like inlining or specialization, if the caller was serialized and the callee would not be serialized.
• Explanation: This is fix to recover the performance regression reported for Beta1.
• Scope of Issue: Compiling a 3rd party code that does not use resilience may result in performance degradation, because some optimizations like specialization or inlining could be skipped due to some serialization related checks.
• Origination: rdar://32718853 by an external developer
• Risk: Low. It restricts SIL serialization of witness tables and protocol witness thunks.
This should fix rdar://32718853