[libc++] First attempt to regroup a few modules in the modulemap #98214
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesWe split up all the headers into top-level modules when we broke up cycles with the C compatibility headers. However, this resulted in a large number of small modules, which is awkward and clearly against the philosophy of Clang modules. This was necessary to make things work. This patch regroups a few headers from two leaf modules: stop_token and pstl. It should be pretty uncontroversial that grouping these headers into a single module doesn't introduce any cyclic dependency, yet it's a first step towards reducing the number of top-level modules we have in our modulemap. Full diff: https://github.com/llvm/llvm-project/pull/98214.diff 4 Files Affected:
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 9ffccf66ff094..59ad740ac164a 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -245,8 +245,15 @@ module std_stdexcept [system] {
header "stdexcept"
export *
}
-module std_stop_token {
+module std_stop_token [system] {
header "stop_token"
+ private header "__stop_token/atomic_unique_lock.h"
+ private header "__stop_token/intrusive_list_view.h"
+ private header "__stop_token/intrusive_shared_ptr.h"
+ private header "__stop_token/stop_callback.h"
+ private header "__stop_token/stop_source.h"
+ private header "__stop_token/stop_state.h"
+ private header "__stop_token/stop_token.h"
export *
}
module std_streambuf [system] {
@@ -1584,41 +1591,25 @@ module std_private_numeric_transform_exclusive_scan [system] { header "__numeric
module std_private_numeric_transform_inclusive_scan [system] { header "__numeric/transform_inclusive_scan.h" }
module std_private_numeric_transform_reduce [system] { header "__numeric/transform_reduce.h" }
-module std_private_pstl_backend [system] {
+module std_private_pstl [system] {
header "__pstl/backend.h"
- export *
-}
-module std_private_pstl_backend_fwd [system] {
header "__pstl/backend_fwd.h"
- export *
-}
-module std_private_pstl_backends_default [system] {
header "__pstl/backends/default.h"
- export *
-}
-module std_private_pstl_backends_libdispatch [system] {
header "__pstl/backends/libdispatch.h"
- export *
-}
-module std_private_pstl_backends_serial [system] {
header "__pstl/backends/serial.h"
- export *
-}
-module std_private_pstl_backends_std_thread [system] {
header "__pstl/backends/std_thread.h"
- export *
+ header "__pstl/cpu_algos/any_of.h"
+ header "__pstl/cpu_algos/cpu_traits.h"
+ header "__pstl/cpu_algos/fill.h"
+ header "__pstl/cpu_algos/find_if.h"
+ header "__pstl/cpu_algos/for_each.h"
+ header "__pstl/cpu_algos/merge.h"
+ header "__pstl/cpu_algos/stable_sort.h"
+ header "__pstl/cpu_algos/transform.h"
+ header "__pstl/cpu_algos/transform_reduce.h"
+ header "__pstl/dispatch.h"
+ header "__pstl/handle_exception.h"
}
-module std_private_pstl_cpu_algos_any_of [system] { header "__pstl/cpu_algos/any_of.h" }
-module std_private_pstl_cpu_algos_cpu_traits [system] { header "__pstl/cpu_algos/cpu_traits.h" }
-module std_private_pstl_cpu_algos_fill [system] { header "__pstl/cpu_algos/fill.h" }
-module std_private_pstl_cpu_algos_find_if [system] { header "__pstl/cpu_algos/find_if.h" }
-module std_private_pstl_cpu_algos_for_each [system] { header "__pstl/cpu_algos/for_each.h" }
-module std_private_pstl_cpu_algos_merge [system] { header "__pstl/cpu_algos/merge.h" }
-module std_private_pstl_cpu_algos_stable_sort [system] { header "__pstl/cpu_algos/stable_sort.h" }
-module std_private_pstl_cpu_algos_transform [system] { header "__pstl/cpu_algos/transform.h" }
-module std_private_pstl_cpu_algos_transform_reduce [system] { header "__pstl/cpu_algos/transform_reduce.h" }
-module std_private_pstl_dispatch [system] { header "__pstl/dispatch.h" }
-module std_private_pstl_handle_exception [system] { header "__pstl/handle_exception.h" }
module std_private_queue_fwd [system] { header "__fwd/queue.h" }
@@ -1773,23 +1764,6 @@ module std_private_span_span_fwd [system] { header "__fwd/span.h" }
module std_private_stack_fwd [system] { header "__fwd/stack.h" }
-module std_private_stop_token_atomic_unique_lock [system] { header "__stop_token/atomic_unique_lock.h" }
-module std_private_stop_token_intrusive_list_view [system] { header "__stop_token/intrusive_list_view.h" }
-module std_private_stop_token_intrusive_shared_ptr [system] { header "__stop_token/intrusive_shared_ptr.h" }
-module std_private_stop_token_stop_callback [system] { header "__stop_token/stop_callback.h" }
-module std_private_stop_token_stop_source [system] {
- header "__stop_token/stop_source.h"
- export *
-}
-module std_private_stop_token_stop_state [system] {
- header "__stop_token/stop_state.h"
- export *
-}
-module std_private_stop_token_stop_token [system] {
- header "__stop_token/stop_token.h"
- export *
-}
-
module std_private_string_char_traits [system] {
header "__string/char_traits.h"
export *
diff --git a/libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp b/libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp
index 2a9b828f4389c..bd08ac17e9964 100644
--- a/libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp
+++ b/libcxx/test/libcxx/thread/thread.stoptoken/atomic_unique_lock.pass.cpp
@@ -12,7 +12,7 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17
-#include <__stop_token/atomic_unique_lock.h>
+#include <stop_token>
#include <atomic>
#include <cassert>
#include <chrono>
diff --git a/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_list_view.pass.cpp b/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_list_view.pass.cpp
index 85cd978625895..35b96c0d84fea 100644
--- a/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_list_view.pass.cpp
+++ b/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_list_view.pass.cpp
@@ -9,7 +9,7 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17
-#include <__stop_token/intrusive_list_view.h>
+#include <stop_token>
#include <cassert>
#include "test_macros.h"
diff --git a/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_shared_ptr.pass.cpp b/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_shared_ptr.pass.cpp
index 47440015f2c50..3ba8f484d0c64 100644
--- a/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_shared_ptr.pass.cpp
+++ b/libcxx/test/libcxx/thread/thread.stoptoken/intrusive_shared_ptr.pass.cpp
@@ -9,7 +9,7 @@
// UNSUPPORTED: c++03, c++11, c++14, c++17
-#include <__stop_token/intrusive_shared_ptr.h>
+#include <stop_token>
#include <atomic>
#include <cassert>
#include <utility>
|
| header "stop_token" | ||
| private header "__stop_token/atomic_unique_lock.h" |
There was a problem hiding this comment.
Shouldn't these still be submodules?
There was a problem hiding this comment.
I don't know? Is there a reason why we should prefer making it a submodule rather than listing private headers like this? Genuine question, I'm not an expert on Clang modules.
There was a problem hiding this comment.
Yes generally every header should be in its own submodule. Otherwise including stop_token when building with modules will implicitly include all of these headers as well, even if stop_token itself doesn't include them.
There was a problem hiding this comment.
But <stop_token> does need these headers.
There was a problem hiding this comment.
Well yes. The stop_token implementation headers are a bit unique in that stop_token itself directly includes all of them, and none of the other headers do. Something more typical is like __type_traits headers. You can't do like this.
module std_type_traits [system] {
header "type_traits"
private header "__type_traits/A.h"
export *
}
- If say
algorithmincludes __type_traits/A.h it will also get type_traits and all of the other headers declared at the same level. algorithmcan't include __type_traits/A.h because it's private.
So while this is fine for stop_token, it's going to unravel fast when you try to do some of the more significant ones.
There was a problem hiding this comment.
This seems like a surprising best practice. Aren't we trying to stop building so many small modules in the first place? The way I understand it:
stop_tokenis a module, and it has both public APIs and private APIs, as denoted by the public/private headers declared in that moduletype_traitsis a module, but it has almost exclusively public headers because__type_traits/xxxxx.hare meant to be included directly from other parts of the code.
Either way -- we don't currently expect other parts of the code to include implementation details of <stop_token>. If they did, I would argue for moving these parts (e.g. intrusive_shared_ptr) to another module that contains generally-useful utilities, or to memory, or something. But obviously <stop_token> should stay a leaf module whatever happens, and in that sense I think it makes most sense to define it the way it is currently defined (with private headers).
There was a problem hiding this comment.
Submodules only control visibility. If you make a submodule for all of the __stop_token headers, it'll still build all of them into a single pcm. It only controls what's visible when something includes __stop_token/atomic_unique_lock.h. i.e. if you have this.
module std_stop_token [system] {
header "stop_token"
module atomic_unique_lock { private header "__stop_token/atomic_unique_lock.h" }
...
}
Then std_stop_token builds the same as it does the way you have it right now and still cuts down on all the module builds/pcms. The difference is that if something includes __stop_token/atomic_unique_lock.h, then it behaves like non-modular C and you only get __stop_token/atomic_unique_lock.h. But the way you have it, if you build with modules then you also get stop_token and all of the other __stop_token headers, which is pretty unintuitive.
There was a problem hiding this comment.
I see, thanks for the clarification. This is clearly relevant for how to setup e.g. type_traits. However, for stop_token, nobody outside of the stop_token module can include __stop_token/atomic_unique_lock.h, right?
There was a problem hiding this comment.
As long as you keep it a private header, yes that's right.
There was a problem hiding this comment.
(I believe the original mega module had submodules for all of the implementation headers like I'm suggesting btw)
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| // UNSUPPORTED: c++03, c++11, c++14, c++17 | |||
|
|
|||
| #include <__stop_token/atomic_unique_lock.h> | |||
There was a problem hiding this comment.
Could we instead just add -Wno-private-header to all libcxx/test/libcxx tests? I think ideally we wouldn't export library details at all other than by including detail headers.
There was a problem hiding this comment.
We can, but I think I'd rather add it on a per test basis to avoid making it too easy to include detail headers unknowingly.
ldionne
force-pushed
the
review/modulemap-cleanup-gradual
branch
from
99edfb4
to
8451f06
Compare
|
I am wondering what prevents us from wrapping all of the content of the modulemap into a Although I do not see a point in having separate pcm files for each header, I see two solutions that can restore piece in our software galaxy:
What do you think? |
The approach you link to has the same problem that caused us to split the modulemap in the first place. It creates a circular dependency if there's a module in the SDK that includes e.g. I'm not convinced that there's any better approach than having a single, simplified modulemap that matches our actual library organization. I'd be happy to change my mind if someone can explain why that can't work or is not desirable, but so far I have not seen any evidence to support that, so I'm still pretty set on pursuing that. |
We split up all the headers into top-level modules when we broke up cycles with the C compatibility headers. However, this resulted in a large number of small modules, which is awkward and clearly against the philosophy of Clang modules. This was necessary to make things work. This patch regroups a few headers from two leaf modules: stop_token and pstl. It should be pretty uncontroversial that grouping these headers into a single module doesn't introduce any cyclic dependency, yet it's a first step towards reducing the number of top-level modules we have in our modulemap.
ldionne
force-pushed
the
review/modulemap-cleanup-gradual
branch
from
8451f06
to
03c7756
Compare
Can't we add |
These tests are only enabled on Apple platforms. The purpose of these tests is to ensure that we don't break existing functionality powered by Clang modules as we refactor the modulemap, since Swift interop greatly exercises libc++'s support for Clang modules. In particular, adding these tests to libc++ is not an official statement of support from libc++ towards Swift interop (which would require a community consensus that I am not pursuing at this time), but rather a way for us to find out when we unexpectedly break things so we can decide how / whether we want to address such breakage. Once the refactoring of the modulemap started in llvm#98214 is mostly done, it would be reasonable to remove these tests unless libc++ wants to pursue support for Swift interop more officially.
|
I just did some experiments with regrouping modules in the exact same way and I managed to get an absolutely gigantic speedup in our clang modules tests (the So I think this approach is fruitful -- I'll merge this and start opening PRs that work towards what I have locally. |
Hi @ldionne. Do you have stats on how many standalone modules we get before and after this PR? |
|
I've just started working my way through the modulemap. For now it only reduces the number of top-level modules by a few since I merged all the private |
Thanks for clarifying. Unfortunately that's not a workable solution for us. We would like to have a way to build a single module as per other discussion threads. What would be a way forward to implement that upstream? I still think that we can use cmake to build a std.modulemap file which wraps the content of the default modulemap. |
A single module can never work. For example libc++'s While we can't have the C compatibility headers in a single top-level module, I actually don't see why we couldn't have the other libc++ headers in a single top-level module. I think that would be doable. |
I tried that, and the public c++ headers were too tangled with the c headers to make that practical. But maybe you'll have better luck. I'm not sure what people are trying to do with modules outside of Swift/Apple environments. There are very few other environments that have any module support at the libc layer so it doesn't really make sense to try to use the libc++ module. I think most people just want pch... |
I am stuck understanding this. We do have one of the best modularized framework infrastructure (claim is entirely mine here). I do not see the problem you are describing. The one I think you might refer to is/was solved by
I am not sure I follow but how single top-level module per header file does not fall into your description for a cycle above?
We do need modules because PCH does not scale beyond 512 Mb due to the offsets in the AST reader. I'd like to extend that discussion by pinging @ChuanqiXu9, @zygoloid, @Bigcheese and @bcardosolopes. There must be something that I or you guys are missing... |
This sounds like a problem in how the modules are set up for the base system. Stuff that libc++ depends on cannot be in the same module as stuff that depends on libc++. That would be broken layering, and splitting up the libc++ module would be working around that, not fixing it. |
The libc++ wrapper headers are presumably going to make fixing this hard. Potentially one path forward here might be to set up the base system module so that its references to (for example) "stddef.h" include the "stddef.h" from the base system, not the wrapper header from libc++. I don't recall if there's a direct way to express that in the module map for the base system currently, but that seems like something we could add if not. [Edit:] Maybe that'd cause problems too. If libc++'s wrapper header is (for example) undefining macros from stddef.h, then it'd be bad for the base system's math.h to include its own stddef.h directly because those macros would then leak, not from stddef.h, but from math.h. Maybe another option: split the base system up into separate top-level modules for parts that are below libc++ and parts that are above libc++, and do not define modules in C++ for the headers that are wrapped by libc++ (allowing the libc++ modules to fully handle those headers) -- or treat those headers as textual headers. |
|
There's just no good way to deal with Modules aren't a good solution to a size deficiency in PCH, it sounds like that just needs to be fixed. |
|
But also, undef'ing doesn't work in modules. In other words, you can't import and module and then start undef'ing its macros. They're still going to be there for everyone else that imports the original module even if they import your module. At least I'm pretty sure the undef will only have an effect for your module... |
|
It's possible that |
|
Hmm, we had to make __undef_macros a textual header to get that to work. |
For what it's worth, what we do in Google's modules deployment is to have a layer of wrapper headers that sits between the application code and the base system (libc, libc++, etc), and we build a module for that layer of wrapper headers (and nothing below it). That works great for us. So there are other options here :)
This isn't just a size issue. Various things in Clang scale linearly in the number of PCM files loaded. The C++ standard library currently has over 120 public headers (including both Perhaps something that would be interesting to explore would be putting multiple top-level modules in the same PCM file. While splitting a single top-level module across PCM files has some issues, I don't think there's any real problems (other than some hardcoded assumptions and the default behavior of various PCM search mechanisms) with putting multiple modules in the same PCM file. If the base system's module map could declare that the |
There is a good way to deal with this:
Yes, they are. We cannot fix the size deficiency in the PCH because it is a space constraint and will penalize people with smaller PCH sizes. Modules give us layering pretty much the same reason for why Apple uses it (along with other things). I will keep asking for a case of a cycle which is not fixed by |
Yes the current setup is pretty crummy and really slow too. It's not only the public headers that have their own top level modules, but all of the implementation headers too. We've explored paring that down and can get it under 200 algorithmically, but Louis is looking at doing it by hand and fixing some unnecessary dependencies along the way.
That is an interesting idea. Yeah Apple doesn't have LSV on because it doesn't support Objective-C (it doesn't see ObjC categories or protocols so pretty much nothing compiles). |
|
Generally speaking |
We split up all the headers into top-level modules when we broke up cycles with the C compatibility headers. However, this resulted in a large number of small modules, which is awkward and clearly against the philosophy of Clang modules. This was necessary to make things work.
This patch regroups a few headers from two leaf modules: stop_token and pstl. It should be pretty uncontroversial that grouping these headers into a single module doesn't introduce any cyclic dependency, yet it's a first step towards reducing the number of top-level modules we have in our modulemap.