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

Add TSS (Thread Specific Storage) API in CPython 3.7+ #1939

Merged
merged 3 commits into from Oct 31, 2017

Conversation

@sonots
Copy link
Contributor

commented Oct 16, 2017

For #1932

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

I will try to create a compatibility layer separately.

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

I wrote compatibility layer codes as follows, but I am pretty not sure where I shout put this:

#if PY_VERSION_HEX <= 0x030700A1
// TSS (Thread Specific Storage) API in CPython 3.7+

#define Py_tss_NEEDS_INIT 0
typedef int Py_tss_t;
int PyThread_tss_create(Py_tss_t *key) {
  // PyThread_create_key will report initial success,
  // but attempts to use the returned key are likely to fail.
  *key = PyThread_create_key();
  return 0;  // always success
}
Py_tss_t * PyThread_tss_alloc(void) {
  int *key = malloc(sizeof(Py_tss_t));
  *key = Py_tss_NEEDS_INIT;
  return key;
}
void PyThread_tss_free(Py_tss_t *key) {
  free(key);
}
int PyThread_tss_is_created(Py_tss_t *key) {
  return *key != Py_tss_NEEDS_INIT;
}
void PyThread_tss_delete(Py_tss_t *key) {
  PyThread_delete_key(key);
  *key = Py_tss_NEEDS_INIT;
}
int PyThread_tss_set(Py_tss_t *key, void *value) {
  return PyThread_set_key_value(key, value);
}
void * PyThread_tss_get(Py_tss_t *key) {
  return PyThread_get_key_value(key);
}
// PyThread_delete_key_value(key) is equalivalent to PyThread_set_key_value(key, NULL)
// PyThread_ReInitTLS() is a no-op

#endif
@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2017

I would change the preprocessor guard to
#if PY_VERSION_HEX <= 0x030700A1 && !defined(PyThread_tss_create)
Eventually, PyPy might also support this, and they will usually have #defines for the C-API functions.

And then I think it should go right into the preamble section in ModuleSetupCode.c, maybe after the PyThreadState adaptation code near line 260. Note that all functions must be static.

Also look at the GIL handling code at the end of that file. It might need adaptation to use the new API if available. (Not a requirement, it's not really related to this change.)

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

I've added the compatible layer codes at the bottom of ModuleSetupCode.c because I wanted to use CYTHON_INLINE and it was defined at line 536.

I am still not sure how to use the codes. I wrote a hello.pyx as:

from cpython.pythread cimport *

cdef Py_tss_t tss_key = Py_tss_NEEDS_INIT
if PyThread_tss_create(&tss_key):
    pass

cdef int the_value = 1;
if PyThread_tss_get(&tss_key) == NULL:
    PyThread_tss_set(&tss_key, <void *>&the_value)

PyThread_tss_delete(&tss_key);

cdef Py_tss_t *ptr_key = PyThread_tss_alloc()

PyThread_tss_free(ptr_key)
ptr_key = NULL;

Cythonizing succeeds,

cython --embed hello.pyx

but compilation

gcc `python-config --cflags` -o hello hello.c `python-config --ldflags`

fails as:

gcc -I/home/sonots/.pyenv/versions/3.6.2/include/python3.6m -I/home/sonots/.pyenv/versions/3.6.2/include/python3.6m  -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -o hello hello.c -L/home/sonots/.pyenv/versions/3.6.2/lib/python3.6/config-3.6m-x86_64-linux-gnu -L/home/sonots/.pyenv/versions/3.6.2/lib -lpython3.6m -lpthread -ldl  -lutil -lm  -Xlinker -export-dynamic

hello.c:903:8: error: unknown type name ‘Py_tss_t’
 static Py_tss_t __pyx_v_5hello_tss_key;
        ^
hello.c:905:8: error: unknown type name ‘Py_tss_t’
 static Py_tss_t *__pyx_v_5hello_ptr_key;
        ^
hello.c: In function ‘PyInit_hello’:
hello.c:1131:16: warning: implicit declaration of function ‘PyThread_tss_create’ [-Wimplicit-function-declaration]
   __pyx_t_1 = (PyThread_tss_create((&__pyx_v_5hello_tss_key)) != 0);
                ^
hello.c:1151:17: warning: implicit declaration of function ‘PyThread_tss_get’ [-Wimplicit-function-declaration]
   __pyx_t_1 = ((PyThread_tss_get((&__pyx_v_5hello_tss_key)) == NULL) != 0);
                 ^
hello.c:1151:61: warning: comparison between pointer and integer
   __pyx_t_1 = ((PyThread_tss_get((&__pyx_v_5hello_tss_key)) == NULL) != 0);
                                                             ^
hello.c:1161:5: warning: implicit declaration of function ‘PyThread_tss_set’ [-Wimplicit-function-declaration]
     PyThread_tss_set((&__pyx_v_5hello_tss_key), ((void *)(&__pyx_v_5hello_the_value)));
     ^
hello.c:1179:3: warning: implicit declaration of function ‘PyThread_tss_delete’ [-Wimplicit-function-declaration]
   PyThread_tss_delete((&__pyx_v_5hello_tss_key));
   ^
hello.c:1188:28: warning: implicit declaration of function ‘PyThread_tss_alloc’ [-Wimplicit-function-declaration]
   __pyx_v_5hello_ptr_key = PyThread_tss_alloc();
                            ^
hello.c:1188:26: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
   __pyx_v_5hello_ptr_key = PyThread_tss_alloc();
                          ^
hello.c:1196:3: warning: implicit declaration of function ‘PyThread_tss_free’ [-Wimplicit-function-declaration]
   PyThread_tss_free(__pyx_v_5hello_ptr_key);

I guess I need to prepare a header file, but I am not sure where I should put it.

@dalcinl

This comment has been minimized.

Copy link
Member

commented Oct 18, 2017

@sonots Could you add # cython: fast_gil=True at the top of hello.pyx and try again?

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

@dalcinl I got same failure.

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Ah, sorry. I didn't tell you that the utility code files are split into sections that are inserted explicitly by name.

I moved the Python compatibility code into its own section now, so that you have access to CYTHON_INLINE. That had also annoyed me a couple of times already. See e7e923f. Please rebase on latest master and move your code into the PythonCompatibility section, after the thread state code.

@sonots sonots force-pushed the sonots:tss_api branch from 15f5abf to bff0c2a Oct 20, 2017

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Ah, no, the ///////// ... ///////// lines are the headers that split the section. You just cut the FastGil section into two. :)

I meant to move the C implementation here, without adding a new section for it. You can still keep your "TSS API" title as a normal // ... comment.

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Thank you very much! It seems it is working fine now.

My remained question is: How do we define followings in pxd file?

#define Py_tss_NEEDS_INIT {0}

https://github.com/python/cpython/blob/731e18901484c75b60167a06a0ba0719a6d4827d/Include/pythread.h#L130

@sonots sonots force-pushed the sonots:tss_api branch from bff0c2a to ca2df82 Oct 20, 2017

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Changed to // ...

@sonots sonots changed the title Add TSS (Thread Specific Storage) API in CPython 3.7+ [WIP] Add TSS (Thread Specific Storage) API in CPython 3.7+ Oct 20, 2017

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

#define Py_tss_NEEDS_INIT {0}

This is essentially an external variable of type Py_tss_t, i.e.

Py_tss_t Py_tss_NEEDS_INIT

@sonots sonots force-pushed the sonots:tss_api branch from ca2df82 to ea22b2a Oct 20, 2017

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Note that this new code is not currently being used anywhere. Please add a test .pyx file under tests/run/ that exercises it. You can use a (compiled) Python function with doctests for that, which will be executed by the Python test runner. (And please ignore the comment about the fast_gil directive, it's not needed.)

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Thanks! I will add tests.

@sonots sonots force-pushed the sonots:tss_api branch from ea22b2a to 93e712b Oct 20, 2017

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Ah, I got following error for Py_tss_NEEDS_INIT

/home/sonots/.pyenv/versions/3.7-dev/include/python3.7m/pythread.h:130:29: error: expected expression before ‘{’ token
 #define Py_tss_NEEDS_INIT   {0}
                             ^
hello.c:1181:28: note: in expansion of macro ‘Py_tss_NEEDS_INIT’
   __pyx_v_5hello_tss_key = Py_tss_NEEDS_INIT;
                               ^
@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Hmm, could you try structuring it like this?

#if PY_VERSION_HEX < 0x03070000
#include "pythread.h"
#ifndef Py_tss_NEEDS_INIT
#define Py_tss_NEEDS_INIT 0
typedef int Py_tss_t;
...
@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

The error is occuring for Python 3.7-dev, so it is probably not related 🤔 ?

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Is that the complete error output that you get or is there more? What's your C compiler?

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

The entire message is as:

hello.c: In function ‘PyInit_hello’:
/home/sonots/.pyenv/versions/3.7-dev/include/python3.7m/pythread.h:130:29: error: expected expression before ‘{’ token
 #define Py_tss_NEEDS_INIT   {0}
                             ^
hello.c:1181:28: note: in expansion of macro ‘Py_tss_NEEDS_INIT’
   __pyx_v_5hello_tss_key = Py_tss_NEEDS_INIT;
                            ^
/home/sonots/.pyenv/versions/3.7-dev/include/python3.7m/pythread.h:130:31: error: expected ‘;’ before ‘}’ token
 #define Py_tss_NEEDS_INIT   {0}
                               ^
hello.c:636:36: note: in definition of macro ‘__Pyx_PyBool_FromLong’
 #define __Pyx_PyBool_FromLong(b) ((b) ? __Pyx_NewRef(Py_True) : __Pyx_NewRef(Py_False))
                                    ^
hello.c:1190:38: note: in expansion of macro ‘Py_tss_NEEDS_INIT’
   __pyx_t_1 = __Pyx_PyBool_FromLong((Py_tss_NEEDS_INIT == 0)); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 15, __pyx_L1_error)
                                      ^
hello.c:1190:56: error: expected ‘)’ before ‘==’ token
   __pyx_t_1 = __Pyx_PyBool_FromLong((Py_tss_NEEDS_INIT == 0)); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 15, __pyx_L1_error)
                                                        ^
hello.c:636:36: note: in definition of macro ‘__Pyx_PyBool_FromLong’
 #define __Pyx_PyBool_FromLong(b) ((b) ? __Pyx_NewRef(Py_True) : __Pyx_NewRef(Py_False))
                                    ^
Makefile:8: recipe for target 'hello' failed

compiler is

$ gcc --version
gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

The hello.pyx is as of #1939 (comment)

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Struct initialisations are only allowed in the variable declaration. Use
cdef Py_tss_t tss_key = Py_tss_NEEDS_INIT

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Well, I am doing so ... 🤔
#1939 (comment)

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

Ah, ok. It's a global variable that Cython does not initialise statically. It should still work for a local variable. That's annoying...

Could be something to improve in CPython. Requiring static initialisation doesn't seem ideal.

BTW:

The error is occurring for Python 3.7-dev, so it is probably not related

But it's still the right way to do it ;)

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

Ah, okay, so cython currently generates c codes as follows:

Py_tss_t __pyx_v_tss_key;
__pyx_v_tss_key = Py_tss_NEEDS_INIT;

But, struct initialization must be written as:

Py_tss_t __pyx_v_tss_key = Py_tss_NEEDS_INIT;

or

Py_tss_t __pyx_v_tss_key;
__pyx_v_tss_key = (Py_tss_t)Py_tss_NEEDS_INIT;
@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

If the last works, then CPython should totally use a cast in the macro.

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

I've sent them a pull request: python/cpython#4060

return 0; // PyThread_create_key reports success always
}
static CYTHON_INLINE Py_tss_t * PyThread_tss_alloc(void) {
int *key = malloc(sizeof(Py_tss_t));

This comment has been minimized.

Copy link
@scoder

scoder Oct 20, 2017

Contributor

C++ requires a cast for the malloc() result. I actually think that this should use PyObject_Malloc() and PyObject_Free() instead, which are faster for small chunks of memory.

This comment has been minimized.

Copy link
@scoder

scoder Oct 20, 2017

Contributor

BTW, I think this should be Py_tss_t * and not int *. It's just cleaner.

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2017

Added tests/compile/tss.pyx, but I am not sure whether this is enough.

@sonots sonots force-pushed the sonots:tss_api branch 2 times, most recently from 433bba7 to 9c957a6 Oct 24, 2017

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2017

Please move the test code into a def function. Then you can use a doctest to run it.
Replace mode: compile with mode: run and move the file into /tests/run/.

@scoder scoder added this to the 0.28 milestone Oct 29, 2017

@sonots sonots force-pushed the sonots:tss_api branch 3 times, most recently from 7d4576b to 7869215 Oct 31, 2017

@sonots sonots force-pushed the sonots:tss_api branch from 7869215 to 4920f70 Oct 31, 2017

@scoder scoder added the feature label Oct 31, 2017

@scoder

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2017

Beautiful. Thanks!

@scoder scoder merged commit 4ca8669 into cython:master Oct 31, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@sonots sonots deleted the sonots:tss_api branch Oct 31, 2017

@scoder

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2018

Note: I promoted Py_tss_t to a "real" known Cython type in 8fab3aa. That avoids any static initialisation issues with Py_tss_NEEDS_INIT as Cython can now do that internally as part of the variable declaration.

@sonots

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

Good to know, thanks!

@sonots sonots changed the title [WIP] Add TSS (Thread Specific Storage) API in CPython 3.7+ Add TSS (Thread Specific Storage) API in CPython 3.7+ Apr 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.