oneapi-src / oneTBB

oneAPI Threading Building Blocks (oneTBB)
https://oneapi-src.github.io/oneTBB/
Apache License 2.0
5.59k stars 1.01k forks source link

Reintroduce public headers for various components #320

Open Lastique opened 3 years ago

Lastique commented 3 years ago

After the oneTBB 2021.1.1 release, certain library components were moved into implementation detail headers. The particular components we are using are:

Currently, we have to include implementation detail headers to obtain these components. They may be available indirectly through other public headers, but it is not obvious which, and the public headers add unnecessary code that increases compile times.

alexey-katranov commented 3 years ago
  • Exceptions (e.g. user_abort). Required to be able to catch TBB exceptions in user's code.

tbb::user_abort is a part of the tbb::concurrent_bounded_queue interface. Try to include <tbb/concurrent_queue.h>.

  • Range split tags (e.g. split). Required to be able to implement custom blocked range types.

tbb::split is defined in parallel algorithm and range related headers. See: https://spec.oneapi.com/versions/latest/elements/oneTBB/source/algorithms/split_tags/split_cls.html

The workaround is to include <tbb.h>.

Lastique commented 3 years ago

As I said in the original post, the point of having fine grained includes is to reduce compile times. Which is why I'm asking the dedicated headers and don't want to use tbb.h.

alexey-katranov commented 3 years ago

Does not <tbb/concurrent_queue.h> and <tbb/blocked_range.h> suit your needs? Or instead of <tbb/concurrent_queue.h> would you like to have something like <tbb/exceptions.h> that lists all TBB exceptions?

Lastique commented 3 years ago

Does not <tbb/concurrent_queue.h> and <tbb/blocked_range.h> suit your needs?

No, because I don't need neither concurrent_queue nor blocked_range types where I need exceptions or split.

Or instead of <tbb/concurrent_queue.h> would you like to have something like <tbb/exceptions.h> that lists all TBB exceptions?

Yes, that would be better. Ideally, I'd be happy if there was a header per exception, e.g. <tbb/exceptions/user_abort.h>, and <tbb/exceptions.h> that includes all of them.

alexey-katranov commented 3 years ago

user_abort implementation is about 5-6 lines in header files. Moving it in a separate header file will add about 25-30 lines of service code (license, includes, namespaces, ...). So, it does not solve the initial problem that we want to reduce compile time because in fact we increase the code base.

I would preferer balancing between fine grained headers and overheads that they introduce (e.g. touching too many files during the compilation is also an issue). It seems the current <tbb/detail/_exceptions.h> state is relatively balanced: it provides 4 useful declaration (25 lines) for 85 lines of overall size (compare with 4 files, each 30 lines + service header file (~40 lines) for internal needs).

As for split, I agree that it does not reasonable to include blocked_range while you are not using it.

Lastique commented 3 years ago

user_abort implementation is about 5-6 lines in header files. Moving it in a separate header file will add about 25-30 lines of service code (license, includes, namespaces, ...). So, it does not solve the initial problem that we want to reduce compile time because in fact we increase the code base.

Comments don't count, as they don't increase compiler times that much. Same with namespace open/close. What you win by placing user_abort into a header of its own is avoid including <new> and <stdexcept> and their dependent headers (especially the latter one, as it pulls std::basic_string and std::allocator) because they are not needed for user_abort definition.

However, as I said, <tbb/exceptions.h> would be an improvement and an acceptable solution.

Also, besides split, don't forget proportional_split, which should also be in the dedicated public header with split.

alexey-katranov commented 3 years ago

avoid including <new> and <stdexcept>

Thank you for pointing to that. It seems we can think about public header for exceptions only and a separate header for internals where <new> and <stdexcept> are really required.

Also, besides split, don't forget proportional_split, which should also be in the dedicated public header with split.

Sure.

anton-potapov commented 2 years ago

@Lastique , what is the gain in terms of compile time you see on the environment used ?

isaevil commented 1 year ago

@Lastique is this issue still relevant for you? Could you please respond?

Lastique commented 1 year ago

As far as I can see, fine grained headers were not added yet, not in the latest TBB release. So yes, this is still relevant.

isaevil commented 1 year ago

@Lastique did you have a chance to check how noticeable the improvement of compile time in your environment as Anton asked?

Lastique commented 1 year ago

No, I did not perform the test.

arunparkugan commented 1 month ago

@Lastique is this issue is still relevant?

Lastique commented 1 month ago

@Lastique is this issue is still relevant?

As you can see in the source, the fine-grained headers are still missing. So, yes, it is still relevant.