[libcxx-commits] [PATCH] D103198: [libc++] Add a CI configuration where we test the PSTL integration

Fri Jan 13 09:22:22 PST 2023

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I just had a discussion with @philnik about this patch. I have come to think that attempting to use the shared version of pstl inside libc++ was not a good idea to begin with.

Standard libraries have subtly different requirements and ways of doing things. For example, we use an ABI tag on our implementation-detail functions so that we have the freedom to change them however we please from release to release. Last I checked, I think libstdc++ handles this differently by not making pre/post condition changes even in implementation-detail functions. Those are both valid ways of doing things and I am not criticizing, however trying to share *header* code between the two standard libraries will surface these kinds of fundamental differences, some of which can't be reconciled with any amount of macros. Another example is circular header dependencies -- without being able to include implementation-detail headers of libc++ from the pstl, we may not be able to get rid of some circular dependencies, and we certainly won't be able to achieve the same level of granularity that we have in libc++. Yet another example is how we deal with removing incidental transitive includes -- we have a process for doing it, but it's not clear to me how we'd establish this in a shared pstl library.

In other words, I think that sharing header-level APIs between standard libraries is not a good idea. Sharing implementation that lives in a dylib or static archive is 100% fine since we can treat it as a third-party dependency. Sharing a well-defined backend implementation is also probably fine, but sharing headers is a different can of worms.

My opinion about this is even stronger after seeing what the decision of using an external code-base in our headers has given us so far -- not much. Because of the complexity of integrating pstl properly into libc++, we've basically been sitting on this and a few other difficult-ish questions ever since it was contributed, leading to us not shipping anything for years. Instead, if we had this code directly in libc++, I am confident we would have shipped *something* at this point and started making improvements to it.

@philnik I would suggest we abandon this revision and instead integrate the pstl directly into libc++, marking it as an experimental feature per our usual conventions. We should strive not to touch the backends too much so as to make it easy (but not necessarily automatic) to keep that in sync with the actual pstl code. That's where 90% of changes happen anyway. Rough suggested path:

- Copy over as-is to libc++
- Add a CI job that enables `_LIBCPP_HAS_PARALLEL_ALGORITHMS` -- no big CMake changes needed
- Clang-format the public API, but don't touch the back-ends since we're going to try to share that
- Apply libc++ macros as needed (e.g. `_LIBCPP_HIDE_FROM_ABI`)
- Use the right namespaces
- Get rid of `pstl_config.h` in favour of `<__config>`
- Granularize public API headers
- Now we should be able to simplify the glue like `<__pstl_algorithm>` inclusion to instead include our own granularized headers from `<algorithm>`
- We should be able to drop the custom `_LIBCPP_HAS_PARALLEL_ALGORITHMS` and use our existing machinery for experimental features
- We should also import the tests, since right now we're not testing anything

Doing that, we should strive to maintain the clear boundary between the backends and the public API. The public API is ours and we can change it -- the backend is where the real "work" is, and we want to share that with the pstl as much as possible since folks are making updates and improvements to it.

@rodgert This repo is not going to go anywhere, so this will not have an impact on your ability to keep taking updates made to pstl. We (libc++) will just start porting changes from pstl into libc++ manually when they happen (which isn't *that* often so far).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103198