[clang][Modules] PCM Sizes Increased Significantly After Delta Encoding Optimization Removed

[qiongsiwu]

Hi @hokein @ChuanqiXu9 !

I work with @Bigcheese and @cyndyishida on Clang Modules. We noticed that the PCM sizes have increased significantly (5%) after #145670. I can see that the 5% increase is expected, but combining this with some other commits that increase PCM sizes, we are experiencing build failures where builders run out of disk space.

Do we have a plan going forward to claw back some of the disk space increase caused by this PR?

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Qiongsi Wu (qiongsiwu)

Hi @hokein @ChuanqiXu9 !

I work with @Bigcheese and @cyndyishida on Clang Modules. We noticed that the PCM sizes have increased significantly (5%) after #145670. I can see that the 5% increase is expected, but combining this with some other commits that increase PCM sizes, we are experiencing build failures where builders run out of disk space.

Do we have a plan going forward to claw back some of the disk space increase caused by this PR?

[shafik]

CC @rnk @AaronBallman @erichkeane

[shafik]

Can you identify what are the other commits that increase PCM size?

[shafik]

I am going to point out that we do try to take feedback on PRs pretty seriously, especially from key stakeholders but I will additionally say modules PRs usually don't get a lot of feedback outside a small set of people. I would love to see a wider set of folks regularly reviewing modules PRs and maybe this will allow us to address these types issues more effectively earlier in the process.

[qiongsiwu]

Thanks for chiming in @shafik !

Can you identify what are the other commits that increase PCM size?

d3daa3c causes 3% size increase for some of our projects. In total, these two commits are causing an 8% increase, which becomes significant when added together.

[AaronBallman]

I'm not aware of active plans to claw that memory back, but it would certainly be welcome effort to improve compile time overhead, build artifact size, etc.

[qiongsiwu]

Thanks @AaronBallman !

@hokein other than code clean up, what was the intention of #145670? Was it preparing for enabling 64 bit source location?

[ChuanqiXu9]

We noticed that the PCM sizes have increased significantly (5%) after #145670. I can see that the 5% increase is #145670 (comment), but combining this with some other commits that increase PCM sizes, we are experiencing build failures where builders run out of disk space.

The problem here is, we thought 5% increase in size is slight but you think it is significant. The direct motivation is, we feel the complexity of the code is hard to read and maintain while we don't get significant benefits from it (we don't feel 5% size of PCMs is significant). @hokein experienced this when he did some changes for 64 bit source location. And I also met similar problems while I was refactoring the encodings for source location. And I feel it will be a problem too when we try to implement other refactorings. So from the perspective of maintainability, I think it is better to remove the optimization.

[hokein]

+1 on what @ChuanqiXu9 said. The 5% size increase was an intentional trade-off for long-term code maintainability and potential performance (faster module loading time). We saw this internally at Google as well and accepted the cost.

A cumulative 8% increase is not small. As a short-term mitigation, is it possible to increase the disk space on your builders? It might be the quickest way to get your builds working again.

@hokein other than code clean up, what was the intention of #145670? Was it preparing for enabling 64 bit source location?

The main driver was code simplicity. It wasn't a hard blocker for 64-bit source locations. While fixing a bug I found in the delta encoding, we realized that part of the code was hard to understand and maintain (having two different encoding codepaths). We decided that removing the optimization was a better long-term decision for code health than continuing to maintain that complexity.

[cyndyishida]

The problem here is, we thought 5% increase in size is slight but you think it is significant.

Yes, this is precisely what we want to resolve. To provide some more context on why we consider this significant. Many or most projects that build against Apple SDKs use clang modules. The Apple SDKs contain thousands of individual libraries that all have their own module, resulting in pcms. The average project (transitively) imports hundreds of pcms. If a project is comprised of several targets that pass different compiler options that constitute a different AST, that increases the pcm count. You can imagine how this 5% starts adding up. We identified this problematic commit because it actually caused a couple of projects to run out of disk space in a production enviornment to build an Apple platform. This has an impact on CI, production, and desktop environments for really any Xcode project that is using a default configuration. This is also generally true for any client project building Swift sources. Projects using precompiled headers also suffer.

It would be valuable to figure out if

1. There's a different way to eat the cost of losing this size optimization
2. Keep the optimization in a way that it doesn't oppose code maintainability

I understand that this is easier said than done, but this change is significant enough for our preexisting workflows relying on modules to do so without some kind of mitigation plan.