[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D136624#3900051 <https://reviews.llvm.org/D136624#3900051>, @jansvoboda11 wrote:

>> - Is there a change in cycles/instructions when the module cache is hot? (presumably the common case)
>
> I didn't notice this (but didn't look for it specifically). How could that affect performance for PCM writes?

It wouldn't check write perf, but it's part of the overall build perf impact. This being neutral (no regression) could help to justify landing the change even when there's a penalty for a cold modules cache. My interest in (implicitly-discovered) explicitly-built modules was similar (if that's neutral, then a regression here is less critical).

Partly, trying to dig into why read speeds got slower. But maybe that was noise that went away though when you switched to cycles/instructions?

> Loaded locations make up 68% of all calls to `getAdjustment()`, so it's good it's the first thing we check for. Around 4% of all calls are for locations before the first non-affecting module. That's the second special case. Around 28% are pointing after the last non-affecting module, and the current revision has a special case for that as well. I think it would make sense to prioritize this check. Only the remaining 0.3% of calls do the actual binary search.

Great; looking forward to seeing new numbers.

(BTW, if you check before/after last non-affecting module, does one of those subsume the is-loaded check entirely? Looking at `isLoadedOffset()` makes me think it might. If so, maybe you can replace that check with an assertion.)

In D136624#3900051 <https://reviews.llvm.org/D136624#3900051>, @jansvoboda11 wrote:

>> - Are the PCMs bit-for-bit identical now when a non-affecting module is added to the input? (If not, why not?)
>
> They are not in the current revision, but I'll create a patch that makes them so (we also need to adjust the `NumCreatedFileIDs`).

Great. Seems like another motivation to land this change (despite a regression), as it can impact meta-build perf if/when artifacts are being archived/shared in a larger context. Specifically, if the PCM artifacts are more stable, then:

- They take up less aggregate space in CAS storage.
- Later build steps get more cache hits.

Overall this patch seems like a good step to me; I don't think 1-2% for a single compilation on a cold cache is that bad, assuming a hot cache doesn't regress:

- Across a single build, the cache gets hot pretty quickly as most compilations reuse the same modules.
- Across incremental builds, most of the cache (at least system modules) will (usually) be hot.
- For explicit builds, it sounds like there's no regression anyway.



================
Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833
+  bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const {
+    return SLocOffset >= CurrentLoadedOffset;
+  }
----------------
The logic for `isLoadedOffset()` suggests that it could maybe be subsumed with "location past the end"?


================
Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32
+
+// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > This is exercising the code, but it could do one better and check if the output PCMs are bit-for-bit identical when we (now) expect them to be.
> > > 
> > > Maybe you could do this by having two run lines: one that includes `-I %t/second` and another that doesn't. Then check if the output PCMs are equal.
> > (Or if the PCM isn't bit-for-bit identical yet, maybe at least the AST block should be...)
> Yes, I'll probably drop this test entirely and just check the PCM files are bit-for-bit identical when a non-affecting file is not loaded at all.
That sounds great.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136624/new/

https://reviews.llvm.org/D136624