[RF] Remove safeDeleteList functionality of RooAbsCollection

[gartrog]

This is presumbaly a bit controversial.

safeDeleteList remove elements in order in a RooAbsCollection,
starting with the ones that only have clients and no servers.

This is a slow process, and takes 25% of CPU time on large workspace
manipulation workflows, as it takes place at each workspace::import
call. It can also lead to slow ~RooWorkspace.

The point is, I don't think this logic is needed at all.
~RooAbsArg takes care of properly breaking all the client-server links,
both uplinks and downlinks, for every object. I couldn't find a logical
case where a crash would occur if the safeDeleteList logic were to be
removed.

All RooFit tests pass after this patch. No problem for my heavy
workspace manipulation worflows either.

[phsft-bot]

Can one of the admins verify this patch?

[hageboeck]

If it works, it's all-the-more good to have complicated destructor magic out of the way.

[hageboeck]

@phsft-bot build

[phsft-bot]

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

[hageboeck]

The point is, I don't think this logic is needed at all. ~RooAbsArg takes care of properly breaking all the client-server links, both uplinks and downlinks, for every object. I couldn't find a logical case where a crash would occur if the safeDeleteList logic were to be removed.

All RooFit tests pass after this patch. No problem for my heavy workspace manipulation worflows either.

By now, I think that the breaking of the links came after this destructor magic. You might be right that this is not needed, any more. I suggest to put it in master and see if the address sanitizer build blows up.

[phsft-bot]

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

• projectroot.runtutorials.tutorial_math_ChebyshevPol

[phsft-bot]

Build failed on mac11/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

• projectroot.roottest.python.distrdf.dask.roottest_python_distrdf_dask_dask_test_backend
• projectroot.roottest.python.distrdf.dask.roottest_python_distrdf_dask_dask_test_include_headers
• projectroot.roottest.python.distrdf.spark.roottest_python_distrdf_spark_spark_test_backend
• projectroot.roottest.python.distrdf.spark.roottest_python_distrdf_spark_spark_test_include_headers
• projectroot.bindings.experimental.distrdf.test.pyunittests_distrdf_unit_test_callable_generator
• projectroot.bindings.experimental.distrdf.test.pyunittests_distrdf_unit_test_node
• projectroot.bindings.experimental.distrdf.test.pyunittests_distrdf_unit_test_proxy
• projectroot.bindings.experimental.distrdf.test.backend.pyunittests_distrdf_unit_backend_test_dist
• projectroot.bindings.pyroot.pythonizations.test.pyunittests_pyroot_pyz_rdataframe_makenumpy
• projectroot.bindings.pyroot.pythonizations.test.pyunittests_pyroot_pyz_rtensor

[guitargeek]

Hi @gartrog, thanks for this! I agree that the order doesn't matter, as the RooAbsArg destructor is taking care that there are no dangling pointers in the client and server list.

If the ASAN build doesn't complain after merging this, the PR should also be backported to 6.26. It would be a waste to have this nice speedup not in the release.

[guitargeek]

Just one little request: can you please rename safeDeleteList to deleteList now? And adapt the function documentation to make sense even after the "safe" deletion is removed?

[gartrog]

Should be done @guitargeek

[guitargeek]

Thanks a lot!

[guitargeek]

@phsft-bot build

[phsft-bot]

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

[guitargeek]

LGTM, thank you very much! It's about time that this overhead gets removed.