Add CI/wheel builder for the free-threaded build

[lysnikolaou]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.46%. Comparing base (24d3b2e) to head (1123323).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #190      +/-   ##
==========================================
+ Coverage   79.81%   81.46%   +1.64%     
==========================================
  Files          24       24              
  Lines        1635     1737     +102     
  Branches      241      239       -2     
==========================================
+ Hits         1305     1415     +110     
+ Misses        184      178       -6     
+ Partials      146      144       -2     

[lysnikolaou]

The Windows failure is related to pypa/setuptools#4662 and should be fixed in the next setuptools release.

[MatthieuDartiailh]

Thanks. Do you plan to add the global lock as part of this PR ?

[lysnikolaou]

Whatever you prefer @MatthieuDartiailh. I was thinking of adding the global lock as part of another PR so that one is not blocked by the other. Does that work?

Although, if we decide to open a new PR for the global lock, this one should probably be merged without Windows support so there's some kind of CI coverage.

[MatthieuDartiailh]

I think I would prefer to have everything in that PR because I won't publish wheels without it I think.

[lysnikolaou]

I think I would prefer to have everything in that PR because I won't publish wheels without it I think.

Alright, I'll push everything here then.

[lysnikolaou]

@MatthieuDartiailh I just completed doing the implementation of the global lock, where locking happens every time we switch from the Python bindings into C++ land. This does not solve all possible threading issues, cause there's still a few of them in the Python binding implementation (eg switching pointers around in shared PyObjects).

I actually also did a somewhat deeper dive into the code and I think that the following would work nicely:

• Instead of using SharedData and SharedDataPtr, use std::shared_ptr under the free-threaded build. That'll solve any reference counting issues in the C++ implementation.
• Acquire PyObject-specific locks using Py_BEGIN_CRITICAL_SECTION and friends. Since there's no global state in the C++ implementation (as far as I can see, maybe I'm missing something), and every PyObject holds their own kiwi instances, this would solve any reentrancy issues. The same approach should be used for any threading issues in the Python part of the code.

What do you think?

[MatthieuDartiailh]

@MatthieuDartiailh I just completed doing the implementation of the global lock, where locking happens every time we switch from the Python bindings into C++ land.

Thanks @lysnikolaou !

This does not solve all possible threading issues, cause there's still a few of them in the Python binding implementation (eg switching pointers around in shared PyObjects).

I am not sure what you refer to here. Could you point me to an example ? Also do you have an idea how this could be addressed ?

I actually also did a somewhat deeper dive into the code and I think that the following would work nicely:

* Instead of using `SharedData` and `SharedDataPtr`, use `std::shared_ptr` under the free-threaded build. That'll solve any reference counting issues in the C++ implementation.

We do not use an atomic reference counted pointer for performance (the C++ implementation is document as not being thread safe because of it). I had hoped we could enforce enough control on the binding to preserve this property of the C++ code. I think having a global lock on creation and destruction of reference counted C++ resource should be sufficient to guarantee the safety of the code (I am conscious doing it right may not be so easy, but do you agree on the principle ?)
I will also try to run the benchmark when using std_shared_ptr to see what would the penalty be.

* Acquire `PyObject`-specific locks using `Py_BEGIN_CRITICAL_SECTION` and friends. Since there's no global state in the C++ implementation (as far as I can see, maybe I'm missing something), and every `PyObject` holds their own kiwi instances, this would solve any reentrancy issues. The same approach should be used for any threading issues in the Python part of the code.

The C++ code does not have any global state as far as I remember (and I will try to check again).

What do you think?

If the current solution is safe, I will likely merge as is to unblock the situation.

In a second time I will try to look at benchmarks using std::shared_ptr to see how much performance penalty we would hit because of it, to see if it worth moving away from a global library lock.

[lysnikolaou]

I am not sure what you refer to here. Could you point me to an example ? Also do you have an idea how this could be addressed ?

I'm referring to the fact that some objects are not immutable. For example, users can change variable contexts in place with setContext, which does this (after a change of mine to use Py_XSETREF):

PyObject*
Variable_setContext( Variable* self, PyObject* value )
{
	if( value != self->context )
	{
		Py_XSETREF(self->context, cppy::incref( value ));
	}
	Py_RETURN_NONE;
}

This is not thread-safe if Variable objects are shared across threads. In order for it to become safe, a lock needs to be held every time context is read/written. I'm not sure how many more instances of things like this exist, but I'll check.

I think having a global lock on creation and destruction of reference counted C++ resource should be sufficient to guarantee the safety of the code (I am conscious doing it right may not be so easy, but do you agree on the principle ?)

I agree with that in principle, yes, but I'm still not clear on whether more localized locks + std::shared_ptr will be a performance boost over one global lock. My assumption is that it will be, so we need to check it out.

If the current solution is safe, I will likely merge as is to unblock the situation.

I'll check whether there's any more instances of thread-unsafe stuff happening on the Python side and let you know today, so that we can merge this. Thanks a lot for the help!

[lysnikolaou]

@MatthieuDartiailh I could find any more thread unsafe stuff in the Python bindings (other than the one already mentioned above). I just pushed two commits:

• Lock around getting and setting variable context so that there's no thread safety issues when sharing variable objects
• Disable Windows for now (I'll follow up on this once setuptools works correctly)

With these, I think we're good to go!

[MatthieuDartiailh]

Looks like you have some include issues.

[MatthieuDartiailh]

I will merge as is to be able to test the release workflow but I will wait for the setuptools fix to cut a new release.