Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free-threading support #271

Closed
wants to merge 1 commit into from
Closed

Conversation

FFY00
Copy link
Contributor

@FFY00 FFY00 commented Sep 20, 2024

WIP, don't merge.

@FFY00
Copy link
Contributor Author

FFY00 commented Sep 20, 2024

The kwlist const change is not strictly required given how it is currently being used, but it's a good idea to avoid static variables when possible. This way the compiler will error out on bad changes, instead of potentially introducing bugs on free-threading builds.

The rest of the static variables are only used to construct str objects that shouldn't change, so that shouldn't introduce issues on free-threading builds. I don't think it's worth dropping the static modifier, as that would introduce performance hits, though very minor.

@FFY00
Copy link
Contributor Author

FFY00 commented Sep 20, 2024

From my review of the code, I didn't catch anything else that would be problematic on free-threaded builds — there's no global state other than the static variables mentioned above, and I didn't catch any other instances of unsafe borrowed reference usage.

I'll add some multi-threaded tests now.


#if PY_VERSION_HEX >= 0x30d0000 /* Python >=3.13 */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this version check, under free threading is it instead safe to use:

    // dict = PyModule_GetDict(module);
    // round = PyDict_GetItemString(dict, "round");

    round = PyObject_GetAttrString(builtins, "round");

That way same code across all versions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW. There was a bug in the __round__ hook implementation anyway as it didn't accept ndigits argument. I have fixed that and changed to using PyObject_GetAttrString on assumption that is okay. Because have also made other changes for static vs const, then the only change required for PR may well just be adding:

     #ifdef Py_GIL_DISABLED
         PyUnstable_Module_SetGIL(module, Py_MOD_GIL_NOT_USED);
     #endif

Can you rebase your changes against head of develop branch and check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

I will rebase.

@@ -127,7 +127,7 @@ static int WraptObjectProxy_init(WraptObjectProxyObject *self,
{
PyObject *wrapped = NULL;

static char *kwlist[] = { "wrapped", NULL };
char *const kwlist[] = { "wrapped", NULL };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW. Changing this to const instead of static will result in the compiler complaining (but not erroring), with:

src/wrapt/_wrappers.c:2316:45: warning: passing 'char *const[8]' to parameter of type 'char **' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
 2316 |             "OOO|OOOO:FunctionWrapperBase", kwlist, &wrapped, &instance,

since Python doesn't use const in the prototype of PyArg_ParseTupleAndKeywords and similar functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm, I didn't get that warning locally.

@FFY00
Copy link
Contributor Author

FFY00 commented Oct 8, 2024

I think everything that was covered in this PR has now been addressed in develop.

@FFY00 FFY00 closed this Oct 8, 2024
@FFY00
Copy link
Contributor Author

FFY00 commented Oct 8, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants