Skip to content

Commit

Permalink
[libc++][modules] Rewrite the modulemap to have fewer top-level modules
Browse files Browse the repository at this point in the history
This patch rewrites the modulemap to have fewer top-level modules.
Previously, our modulemap had one top level module for each header
in the library, including private headers. This had the well-known
problem of making compilation times terrible, in addition to being
somewhat against the design principles of Clang modules.

This patch provides almost an order of magnitude compilation time
improvement when building modularized code (certainly subject to
variations). For example, including <ccomplex> without a module
cache went from 22.4 seconds to 1.6 seconds, a 14x improvement.

To achieve this, one might be tempted to simply put all the headers in
a single top-level module. Unfortunately, this doesn't work because
libc++ provides C compatibility headers (e.g. stdlib.h) which create
cycles when the C Standard Library headers are modularized too. This
is especially tricky since base systems are usually not modularized:
as far as I know, only Xcode 16 beta contains a modularized SDK that
makes this issue visible. To understand it, imagine we have the
following setup:

   // in libc++'s include/c++/v1/module.modulemap
   module std {
      header stddef.h
      header stdlib.h
   }

   // in the C library's include/module.modulemap
   module clib {
      header stddef.h
      header stdlib.h
   }

Now, imagine that the C library's <stdlib.h> includes <stddef.h>,
perhaps as an implementation detail. When building the `std` module,
libc++'s <stdlib.h> header does `#include_next <stdlib.h>` to get the
C library's <stdlib.h>, so libc++ depends on the `clib` module.

However, remember that the C library's <stdlib.h> header includes
<stddef.h> as an implementation detail. Since the header search paths
for libc++ are (and must be) before the search paths for the C library,
the C library ends up including libc++'s <stddef.h>, which means it
depends on the `std` module. That's a cycle.

To solve this issue, this patch creates one top-level module for each
C compatibility header. The rest of the libc++ headers are located in
a single top-level `std` module, with two main exceptions. First, the
module containing configuration headers (e.g. <__config>) has its own
top-level module too, because those headers are included by the C
compatibility headers.

Second, we create a top-level std_core module that contains several
dependency-free utilities used (directly or indirectly) from the __math
subdirectory. This is needed because __math pulls in a bunch of stuff,
and __math is used from the C compatibility header <math.h>.

As a direct benefit of this change, we don't need to generate an
artificial __std_clang_module header anymore to provide a monolithic
`std` module, since our modulemap does it naturally by construction.

A next step after this change would be to look into whether math.h
really needs to include the contents of __math, and if so, whether
libc++'s math.h truly needs to include the C library's math.h header.
Removing either dependency would break this annoying cycle.

Thanks to Eric Fiselier for pointing out this approach during a recent
meeting. This wasn't viable before some recent refactoring, but wrapping
everything (except the C headers) in a large module is by far the simplest
and the most effective way of doing this.
  • Loading branch information
ldionne committed Sep 16, 2024
1 parent 09e3a36 commit b71b975
Show file tree
Hide file tree
Showing 7 changed files with 2,151 additions and 2,347 deletions.
1 change: 0 additions & 1 deletion libcxx/include/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,6 @@ set(files
__ranges/views.h
__ranges/zip_view.h
__split_buffer
__std_clang_module
__std_mbstate_t.h
__stop_token/atomic_unique_lock.h
__stop_token/intrusive_list_view.h
Expand Down
193 changes: 0 additions & 193 deletions libcxx/include/__std_clang_module

This file was deleted.

Loading

0 comments on commit b71b975

Please sign in to comment.