Add RCP variant: cooperative_intrusive - #2151

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

This is great! Some high level comments and questions:

1. I take it that some code that hooks into this is needed on the binding side. How hard would this be to implement?
2. Is it only valuable to use this for bindings to languages having their own reference count mechanism? I expect so, but I was thinking if it does not degrade performance in general we wouldn't want to have more RCP implementations than necessary.
3. Is subclassing the problem this solves or is this solving other leakages when using e.g. symengine.py ?
4. Tangential: Currently we have two RCP implementations right? symengine's own and Teuchos. Do we need both? Does the Teuchos implementation add any value? If not perhaps we should consider removing it to reduce complexity.

Thanks for reviewing!
_{(It was hard getting formatted blocks into a numbered list in markdown so I put my answers under these awkward looking headers instead)}

1. Binding code

a) minimal binding example

Here's a branch spun out of this this extra commit showing a rather minimal test of what the binding code looks like:
bjodah@15fa4cb

[ 92%] Building CXX object CMakeFiles/min_ext.dir/min_ext.cpp.o
[100%] Linking CXX shared module min_ext.cpython-313-x86_64-linux-gnu.so
[100%] Built target min_ext
+ echo '=== Running consumer smoke test'
=== Running consumer smoke test
+ PYTHONPATH=/tmp/symengine-cooperative-intrusive-consume/consumer
+ python /work/symengine/bin/cooperative_intrusive_consume/smoke.py
cooperative_intrusive python smoke: OK -> x + y
+ echo '=== SUCCESS: cooperative_intrusive install-consume test passed'
=== SUCCESS: cooperative_intrusive install-consume test passed
+ rm -rf /tmp/symengine-cooperative-intrusive-consume

b) getting everything right

Tear down of the process is still a little tricky if we want to avoid warnings from nanobind's refcount checks on exit and also LeakSanitizer, but this prototype (LLM written, beware) python wrapper seems to clear both in my testing:
https://github.com/bjodah/superproj-symengine-cooperative-intrusive/blob/fc9945355ca3dcd7878c49e43629954f2f4983dc/nbsymengine/support/nanobind_module_common.h#L151

2. Performance & 4. multiple RCP implementations.

I've built SymEngine in release mode for all three RCP implementations and ran the pre-existing benchmarks, and two new benchmarks in this commit: bjodah@3d7ba4c

The results (again I must warn up-front that this is LLM written results file) are here:
https://gist.github.com/bjodah/969b64812b394955081092377a7db9a1
(cooperative_intrusive is ~10-20% slower than SymEngine's own RCP, while Teuchos is ~50% slower)

So to me it looks like performance is very competitive (and that is in line with what I've read prior to trying this out, nanobind's author is very performance-aware).

And as for RCP implementations, I think we could retire TeuchosRCP (our vendored version isn't even thread-safe, there's been upstream fixes since but last time I tried to update our in-tree teuchos subset I ran into a myriad of issues and gave up on it). But maybe the discussion and potentially the removal of Teuchos RCP is better had in a separate issue/PR?

3. Leaks

I think subclassing symbols is the one issue we couldn't work around, but I suspect there could be other ways to create cycles (but that's just a feeling, I don't have any concrete examples).

Thanks for your thorough explanation. I have one follow up question. In the setting of a software package eco-system will it be possible to have symengine and symengine.py separate packages or will they have to be built together? I.e. will symengine.py always have to contain a special version of symengine? Perhaps this is not even currently possible.

So symengine.py builds just fine against this branch as is (if we don't register any hooks for incref/decref this RCP behaves like our existing ones).

If we want symeninge.py to take advantage of the cooperative reference counting, we would need to patch symengine.py. I tasked an LLM to adapt this (unconditionally but I think one could even support both kinds of RCP in parallel using conditional compilation / adapt how we render our cython source templates), here's the output:
bjodah/symengine.py@fb8fb39
(again, beware, AI generated, but it looks correct to me and it seems like it solves the leak already, although I would need some more time manually verifying).