[libcxx-commits] [PATCH] D144322: [libc++][Modules] Make top level modules for all C++ headers with OS/clang versions

[Ian Anderson via Phabricator via libcxx-commits]

iana marked 2 inline comments as done.
iana added a comment.

In D144322#4275247 <https://reviews.llvm.org/D144322#4275247>, @dexonsmith wrote:

> In D144322#4274873 <https://reviews.llvm.org/D144322#4274873>, @iana wrote:
>
>> Discussed this some more with @ldionne, @Bigcheese, @vsapsai, @var-const. We don't love this patch for a few reasons.
>>
>> 1. It looks arbitrary what got left in `std` and what got pulled out to top level.
>> 2. It looks arbitrary which private detail headers are in the new `std_abc` modules and which ones are top level modules.
>> 3. Figuring out where the private detail headers go manually is tricky, and possibly difficult to maintain. (I've rebased this a couple of times over the last few months and it keeps adding module cycles that have been tough to resolve. It might be less difficult if the cycles get resolved as the headers are changed, or it might just always be hard.)
>>
>> The two ideas we had to improve the situation are these.
>>
>> 1. Make a top level module for all of the headers. This is the simple approach and, if there aren't any include cycles, will be the easiest way to avoid module cycles. But it makes ~950 top level modules which looks kind of goofy and has a fair chance at sub-optimal performance.
>
> I'd be curious to see just how bad the perf is with implicitly-discovered-and-explicitly-built modules. Maybe it wouldn't be as bad as suspected. And will get faster with the scanning speedups @jansvoboda11 is working on.
>
> - Do we know how deep the build graph would be? (Would we get good parallelism when building?)

I made a rough graph that's not quite accurate but is probably close enough to answer that question. It's way more wide and shallow than I thought it would be. 
F27171262: libcxx-dependencies.gv.pdf <https://reviews.llvm.org/F27171262>
It's not so much parallelism that we're worried about, it's more like launching clang ~200 times to build the algorithm module and later opening 200 pcm files.

> - I doubt lookups will be too slow (but could be wrong...). I don't remember X (X=5? X=12?), but there's a new top-level lookup table created in a module if it depends on X+ other modules transitively that don't have a top-level lookup table. @rsmith implemented this to unblock using top-level modules for fine-grained headers in C headers, which sounds pretty similar to this situation.
>
>> 1. Make top level modules for all the public headers, and programmatically generate the minimum number of top level modules for the private detail headers at build time such that there are no cycles. I'm still working on this one.
>
> A variant on this: put every header in its own *sub*module. It's probably not hard (and independently useful) to have clang (optionally) diagnose cycles between submodules; this way you can ensure cycles don't accidentally get added over time, without needing a separate top-level module for each header. WDYT?

That doesn't work, clang doesn't care if it can interleave the submodules, i.e. it's not mad about algorithm.__algorithm and chrono.__chrono, it cares that anything in algorithm is cyclic with anything in chrono.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322