Page MenuHomePhabricator

[libc++] Reduces the number of transitive includes.
ClosedPublic

Authored by Mordante on Aug 20 2022, 1:35 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Commits
rG8ff2d6af6906: [libc++] Reduces the number of transitive includes.
Summary

This defines a new policy for removal of transitive includes.
The goal of the policy it to make it relatively easy to remove
headers when needed, but avoid breaking developers using and
vendors shipping libc++.

The method used is to guard transitive includes based on the
C++ language version. For the upcoming C++23 we can remove
headers when we want, but for other language versions we try
to keep it to a minimum.

In this code the transitive include of <chrono> is removed
since D128577 introduces a header cycle between <format>
and <chrono>. This cycle is indirectly required by the
Standard. Our cycle dependency tool basically is a grep based
tool, so it needs some hints to ignore cycles. With the input
of our transitive include tests we can create a better tool.
However that's out of the scope of this patch.

Note the flag _LIBCPP_REMOVE_TRANSITIVE_INCLUDES remains
unchanged. So users can still opt-out of transitives includes
entirely.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mordante updated this revision to Diff 454298.Aug 21 2022, 5:10 AM

Attempt to fix Windows.

Mordante added inline comments.Aug 21 2022, 7:21 AM
libcxx/include/chrono
692 ↗(On Diff #454298)

Note this doesn't belong in this patch. It's a test to see whether we can fix the build issues in D128577.

I think this is a great step forward, but it does raise some interesting/annoying questions. I think we can solve all of them, though.

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
19–21
23–26
28–32
34
45–48
50

Instead of this, I would suggest:

  1. Adding a new libcxx-generate-public-header-graph job that produces all the expected.FOO headers using clang --trace-includes
  2. This should allow getting rid of regenerate_expected_results = False/True in libcxx/test/libcxx/transitive_includes.sh.cpp.
  3. Rewrite graph_header_deps.py to use that data instead of the naive grep-based parsing it has right now.

This is fine to do as a follow-up.

libcxx/docs/ReleaseNotes.rst
48–62
libcxx/include/chrono
686 ↗(On Diff #454298)

I would like to investigate this instead:

#if _BUILDING_ON_WINDOWS_DLL // don't require usage of libexperimental cause it doesn't work yet
  virtual ~format_error() noexcept { }
#endif

I'm not sure it will be better, but we should try it out. It seems more localized to me.

libcxx/test/libcxx/transitive_includes/expected.algorithm
1 ↗(On Diff #454298)

This is extremely unfortunate, but I think that we *have* to generate these headers for each language version if we move forward with this patch. Otherwise, we defeat the whole purpose of having this framework in place to prevent transitive include removal regressions.

17 ↗(On Diff #454298)

We should keep the C++20 removals as minimal as possible. You should keep including <iterator>, <utility> and <variant> explicitly from <algorithm> in <= C++20, but drop them in C++23.

Mordante updated this revision to Diff 454879.Aug 23 2022, 10:02 AM

Test Windows build based on ldionne's suggestion.

Mordante updated this revision to Diff 454903.Aug 23 2022, 11:10 AM

Try to fix the build.

Mordante updated this revision to Diff 455144.Aug 24 2022, 3:44 AM

Fix other Windows build.

ldionne added inline comments.Aug 24 2022, 9:09 AM
libcxx/utils/ci/run-buildbot
570 ↗(On Diff #455144)

I think we should define _LIBCPP_INLINE_FORMAT_ERROR_DTOR inside __config instead. This is a general property of clang-cl / DLL builds, not only our CI build. IOW, we should enable the inline dtor if someone builds DLLs using clang-cl at their desk, which wouldn't be the case right now.

Mordante updated this revision to Diff 455283.Aug 24 2022, 10:36 AM

Test another way to fix the Windows build.

Mordante edited the summary of this revision. (Show Details)Aug 25 2022, 10:21 AM
Mordante edited the summary of this revision. (Show Details)Aug 25 2022, 10:24 AM
Mordante updated this revision to Diff 455641.Aug 25 2022, 10:28 AM
Mordante marked 9 inline comments as done.
Mordante edited the summary of this revision. (Show Details)

Addresses review comments.
Removes unrelated changes, these have been moved to other patches.
Rebased on top of D132584, to include the new transitve include test framework.

Mordante published this revision for review.Aug 25 2022, 10:29 AM
Mordante added inline comments.
libcxx/include/algorithm
1902–1906

During the live code review we discussed to move these transitive includes to the end of the file. That will be done in a followup patch.

libcxx/include/chrono
686 ↗(On Diff #454298)

Fixed in D132667 in the suggested way. The changes are removed here.

libcxx/test/libcxx/transitive_includes/expected.algorithm
1 ↗(On Diff #454298)

Solved in D132534 and D132584.

Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 10:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mumbleskates added inline comments.
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
35

Sentence fragment terminates here

libcxx/docs/ReleaseNotes.rst
62–64

Is "the future" here major releases? years? If this deeply affected me I would want to know a lot more about what kind of timeline I was guaranteed, given "regardless of the language version in use".

Assuming we still plan to remove transitive includes at some point I'm OK with this approach, as it gives us a lot more flexibility when to remove them.

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
12
33–34

Would the plan still be to remove transitive includes at some point? If not the includes will probably end up in a huge mess.

35

I think you are missing something here.

Mordante added inline comments.Aug 25 2022, 12:05 PM
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
33–34

I'm not sure. We surely need to remove them when it breaks us, like I did for <chrono>.
The big issue of removal is that may break the packagers who then need to fix it.
If it was only developers I would have less of an issue with the breakage.

We can still remove them without repercussions from C++23 and I want to look at some more TODO removals and use the granularized headers in more places. Some header include concepts or type_traits which can be minimized now.

I'm not sure whether we would get in a huge mess with the current header method.
Do you have any concrete concerns with this approach?

35

Seems I misapplied the suggested changes,thanks.

avogelsgesang added inline comments.
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
31

when do we consider a C++ version as "supported"?
As soon as we compile under that -std=c++XX flag without errors?
As soon as clang flips from -std=c++2b to -std=c++23?
As soon as we implemented all papers from that C++ version?

E.g., would we currently consider C++20 to be supported, despite not having implemented all papers, yet? If our bar is "some papers are implemented", which papers are those? Would we already consider C++23 as supported, given that we have implemented some of the C++23 papers?

72
philnik added inline comments.Aug 26 2022, 7:22 AM
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
33–34

I'm fine with it if they're removed in a release where we don't get that much done that we can ship stable. LLVM 16 will definitely contain ranges and probably format, std::pmr (PRs coming soon), maybe the PSTL (@ldionne friendly reminder to work on D103198) and more. So that's most likely not a good option, but I don't think that there will be huge additions in LLVM 17. LLVM 15 would actually have been a really good release in hind-sight. The main reason we didn't do it then was because of ranges, but that didn't actually happen.

TL;DR I don't think it would be a huge problem if people don't have a really good reason to update (not that you shouldn't keep your tools up-to-date).

My main concern is that we will have to move stuff around to implement the rest of C++17 and C++20. Having

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 14
...
#endif

#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
...
#endif


#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
...
#endif

seems to me like a bad idea. Adding a few includes also isn't that huge of a deal. I'm not saying we should do it right now, but it might be a good idea to say: We'll remove all transitive includes in LLVM 17 (maybe?), so, vendors, please enable _LIBCPP_REMOVE_TRANSITIVE_INCLUDES for your internal testing and file bugs against any libraries/programs that break. That would give vendors lots of time to resolve any issues.

Mordante marked 4 inline comments as done.Aug 27 2022, 3:22 AM

Thanks for all the feedback!

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
12

I agree with removing the only but the other part but the the headers for should remain.

31

C++20 is considered supported, that's why we put ranges and format under special flags.
I left supported here intentionally since it's already defined in another part of our policy.

https://libcxx.llvm.org/UsingLibcxx.html#using-a-different-version-of-the-c-standard

33–34

First of all I hope we can clean up our transitive includes before LLVM 16. Most of our larger headers have been granularized, we need to use them were appropriate.

I initially also considered to remove them fast, but we're not sure how much of effort that will be for vendors. If they have to file bugs against hundreds of packages it may take them a lot of effort. (I expect the transitive removal of algorithm and type_traits will uncover a lot of missing includes.) So I think keeping them when it doesn't "hurt" us might be even beneficial for us. If a vendor needs to fix a lot of packages or file a lot of bugs it will take them more effort to package, which may lead them to take a longer time to adopt a new libc++.

The policy still allows us the move things around when it "hurts" us. So if implementing PMR means we need to remove additional transitive includes we still should do that, but only try to minimize that. Like in this commit I removed chrono since it makes it hard to implement format.

The end goal is to move these transitive includes to the end of the header.

So I would really like to use this policy and when it turns out it doesn't work we can revise it. We explicitly mention we're free to remove them here and in the release notes.

libcxx/docs/ReleaseNotes.rst
62–64

We kind of intentionally don't want to provide any guarantee. We still miss some C++17 features like parallel algorithms and polymorphic allocators. They may cause header cycles we want to break by removing headers.

Technically code not using the proper includes is wrong. But often you "get away" with it. But not including the proper headers makes code less portable. Some code accepted by libstdc++ will be rejected by libc++ since both libraries have different transitive includes. If you want to make sure your code doesn't miss any includes you can use _LIBCPP_REMOVE_TRANSITIVE_INCLUDES to test before it becomes an issue.

Note that in the C++ Standard there are few required headers and some might even be surprising; <regex> doesn't require <string>. Based on the interface of <regex> I'm not convinced it can be implemented (in a practical way) without providing <string>.

Mordante updated this revision to Diff 456104.Aug 27 2022, 3:23 AM

Addresses review comments.

philnik added inline comments.Aug 27 2022, 5:27 AM
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
33–34

I get that it might be some effort for vendors to file bugs and stuff. That's why my proposal is to remove transitive includes at any point when there is a technical problem with keeping them. When we think we removed most/all the transitive includes we want to remove we tell everyone well in advance that we will remove the transitive includes in LLVM ab. If we think that we don't remove many after LLVM 16, we can say that transitive includes will be removed in LLVM 18 or whatever.

While this does result in some work IMO it's worth it. For example, including <string> already only takes 2/3 as long when adding -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES to the command line and I'm certain this can be reduced further. Including <memory> currently adds about 100ms (the compilation took ~385ms).

Mordante added inline comments.Aug 28 2022, 3:24 AM
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
33–34

Let's discuss this further on Discord.

jloser added a subscriber: jloser.Aug 30 2022, 8:01 AM
jloser added inline comments.
libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
72

Nit: this reads a bit weird to me. Maybe s/at their/with their?

ldionne accepted this revision.Aug 31 2022, 9:16 AM

I think this strikes the right balance between unblocking important work, not creating immense blockers for vendors to ship an updated libc++, and providing a way forward for the library to reduce its tech debt.

Per the policy, we might decide to do a major "let's drop all transitive includes in all standard modes" move, however that would be a bit of a project on its own and we would have to contact our main vendors to see what the impact is. This can totally be done, and I might do it a couple months down the road when things are a bit less busy, however right now I think this is the right way forward.

libcxx/docs/DesignDocs/HeaderRemovalPolicy.rst
31

I'll note that we may want to revisit that definition, but it is indeed the current status quo.

33–34

The plan is definitely to eventually remove incidental transitive includes. In fact, the bigger plan would even be to have users migrate to newer standard versions, and perhaps one day drop support for older versions (we can dream, right?).

The policy is worded so that

(1) we try to minimize disruptive breakage for vendors, yet
(2) we still have the ability to remove incidental transitive includes when technically necessary (e.g. the <format> - <chrono> circular dep that brought us here).

I understand that some folks would like to have a clearer ETA for when we're going to remove those includes, however I would like to avoid committing to something specific because we still have a lot of more important things to do for the next few releases.

This revision is now accepted and ready to land.Aug 31 2022, 9:16 AM
philnik accepted this revision.Aug 31 2022, 9:37 AM

I think this strikes the right balance between unblocking important work, not creating immense blockers for vendors to ship an updated libc++, and providing a way forward for the library to reduce its tech debt.

Per the policy, we might decide to do a major "let's drop all transitive includes in all standard modes" move, however that would be a bit of a project on its own and we would have to contact our main vendors to see what the impact is. This can totally be done, and I might do it a couple months down the road when things are a bit less busy, however right now I think this is the right way forward.

Then I'm happy with this patch. I just wanted to make sure that the plan is still to remove the transitive includes and this approach is just there to make it easier for us to continue implementing new features while we're working on removing the transitive includes.

Mordante marked 6 inline comments as done.Aug 31 2022, 10:48 AM

Thanks for all reviews!

This revision was landed with ongoing or failed builds.Aug 31 2022, 10:50 AM
This revision was automatically updated to reflect the committed changes.
Mordante added inline comments.Sep 2 2022, 8:56 AM
libcxx/include/algorithm
1902–1906