Removed option `tbb` is still considered to enable secret optimization of `ROOT::gCoreMutex`

[guitargeek]

Description

A long time ago, ROOT had a tbb build option, but it was removed in the ROOT 6.08 development cycle in dd899a5 from 2015, by @peremato.

However, some build configurations still use it, like CMSSW and Conda:

• https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_15_1_X/master/root.spec#L78
• https://github.com/conda-forge/root-feedstock/blob/main/recipe/build_root.sh#L342

That would not be a problem per se, but then, in 2021, @bendavid introduced a new optimization that is enabled only with the long-time removed tbb flag (or builtin_tbb), in 6c8b77d:

• Add TBB-based counter for global read-write lock #7260

So we ended up with a secret tbb=ON configuration that our CI doesn't test, and a CMake configuration option that has a pretty fundamental effect but is not documented.

Now, this comes back to bite us, because loading libtbb and especially libtbbmalloc when loading libRIO -> libThread -> libtbb can have unintended side effects. The libRIO is always loaded when the interpreter is initialized, and tbb then loads its custom allocator. I think this can result in mismatching allocation/deallocation, when ROOT is imported in Python after importing other Python modules that already used the same shared libraries as ROOT but with other allocators.

The example is this Conda issue here:

• Cannot import tensorflow first and then ROOT with 6.32 (BLAS version mismatch?) conda-forge/root-feedstock#253

Both TensorFlow and ROOT use the libzstd support in LLVM to decompress C++ modules (I think), and switching allocators mid-way might cause the reported crash.

I don't know yet what's the best action here, but for sure we should not have an undocumented build option that changes the behavior of ROOT::gCoreMutex.

[bendavid]

Well I guess the underlying problem here is bad symbol visibility behaviour from Tensorflow (we had tons of problems with this in our analysis code related to symbol conflicts with Eigen that we ended up patching out, but I haven't gotten around to upstreaming this yet: https://gitlab.cern.ch/bendavid/cmswmassdocker/-/blob/f987d251941408a4452f8e95272808d26024cfd1/build/tensorflow/symbol-visibility.patch )

For the tbb loading in ROOT, obviously this shouldn't depend on an undocumented build option, but wouldn't any build with the imt flag trigger the same problematic loading? (ie here:

root/core/imt/CMakeLists.txt

Line 55 in ddc834a

target_link_libraries(Imt PRIVATE ${TBB_LIBRARIES})

)

Ok I guess this is less likely to cause problems, but only because libImt is less likely to be loaded than libThread
(ie even if the tbb flag is off, the problem can still occur if loading of libImt is triggered)

[bendavid]

Also another reason I never noticed this: The tbb flag is also used in the Arch Linux package: https://gitlab.archlinux.org/archlinux/packaging/packages/root/-/blob/e72fd11e8259e1eb2ba9b69144d2b05da90e63aa/settings.cmake#L94

[guitargeek]

Thanks for chiming in! Indeed, by building with imt=OFF, the problem is only delayed, the crashes come probably later when doing your analysis, decompressing some data for RDataFrame.

The CI seems to be fine with your optimization though! So maybe the easiest way forward is to always use the optimized ROOT::gCoreMutex if ROOT was built with imt=ON, because then we depend on tbb already. Then, all standard ROOT builds can benefit from your nice optimization. Or you see any problem with that?

Oh well, Arch has it too okay... That is a good example for why we are nowadays printing configuration errors when removed build options are used 🙂

[guitargeek]

What I like less about coupling it to the imt option is that the mutex has a priori nothing to do with implicit multithreading. Another possibility is to bring the tbb option back, but if it's only affecting one line in the code it's maybe not worth it to have an extra option just for that. To be discussed

[bendavid]

Indeed I don't see any problem having this on by default.

If we really really needed to avoid linking to tbb at this stage the alternative would be to find another way to implement the optimization (effectively requiring to re-implement tbb::enumerable_thread_specific)

[guitargeek]

If we really really needed to avoid linking to tbb at this stage

It's not only about the linking, it's also about having TBB as a required Core dependency of ROOT. It would be nice if TBB is not strictly required when building ROOT.

If we really really needed to avoid linking to tbb at this stage the alternative would be to find another way to implement the optimization (effectively requiring to re-implement tbb::enumerable_thread_specific)

I think this is a great idea! I was about to propose that too. What do you think about this implementation?

• [core] Implement RecurseCountsThreadLocal to be used in gCoreMutex #19799