Created on 2015-11-18 15:50 by EdSchouten, last changed 2017-03-23 10:13 by haypo.
|issue25658-1.patch||erik.bray, 2016-08-30 13:53|
|key-constant-time.diff||EdSchouten, 2016-08-31 15:46||review|
|pythread-key-intptr_t.patch||masamoto, 2016-11-10 15:37||review|
|configure-pthread_key_t.patch||masamoto, 2016-11-19 19:28||review|
|pythread-tss.patch||masamoto, 2016-12-05 09:52||review|
|pythread-tss-2.patch||masamoto, 2017-01-09 12:46||review|
|pythread-tss-3.patch||masamoto, 2017-01-22 15:11||minor changes for version 2||review|
|msg254846 - (view)||Author: Ed Schouten (EdSchouten) *||Date: 2015-11-18 15:50|
While trying to port Python over to a new platform (CloudABI), I noticed a couple of compiler errors in PyThread_create_key(), PyThread_delete_key(), PyThread_delete_key_value() and PyThread_set_key_value() caused by fact that pthread_key_t is converted to an integer (and vice versa) POSIX doesn't seem to require that pthread_key_t is an integer or any other arithmetic type: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html Old revisions of the standard did require pthread_t to be an arithmetic type, but this requirement was dropped later on. In my opinion we should strongly consider changing the API, so that we can treat the key created by pthread_key_create() or returned by TlsAlloc() as an opaque type.
|msg273441 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-23 13:30|
I agree--this has the same problem on Cygwin, where pthread_key_t is not just a typedef'd integer (in fact it's a pointer to an instance of a class). Anyways as Ed wrote above POSIX says this is supposed to be an opaque type and there's no reason to assume it's an integer. Linux could change its definition tomorrow and we'd have no one to blame but ourselves if it breaks Python. If it's too hard to change the Python API, at the very least the change in #22206 should be reverted or reworked somehow, because there's no reason to assume that pthread_key_t can even be compared safely to an integer, much less that it would be less than INT_MAX.
|msg273443 - (view)||Author: STINNER Victor (haypo) *||Date: 2016-08-23 13:31|
What do you suggest? Python C code bases uses "long" everywhere.
|msg273451 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-23 14:13|
I'm not really sure what "long" has to do with it... The problem is that the PyThread API uses ints to represent TLS keys, and has for about as long as it's existed (maybe what you meant by "long"). But when support for native TLS was added (#9786 for pthread, sometime earlier for Windows) , the faulty assumption as made in several places that this API (i.e. the type of key is "int") should always map perfectly onto native APIs, and it doesn't. There are several places for example where an int key is passed to pthread_getspecific and pthread_setspecific (http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_getspecific.html). On Linux the compiler happens to allow this because pthread_key_t is defined as "unsigned int" So yeah there's an implicit cast, but since it counts up from zero it usually works. Likewise TlsAlloc on Windows expects the key to be a DWORD but the compiler will accept an int. This is really an unsafe assumption though, especially when PyThread_create_key casts the key to an int, and later reuses the (possibly not safely cast) key with PyThread_delete_key/get_key_value/set_key_value). This was brought up at the time, where MvL wrote: > In principle, this is really difficult to get right. AFAICT, pthread_key_t doesn't even have to be an integral type, and even if it is, it may well become -1. However, we can probably worry about this when a system comes along where this implementation breaks. One possible workaround without changing the existing API would be this: Each native support wrapper should also provide a *safe* mapping between its native key types and ints, to support the PyThread API. For example, the pthread interface could maintain a linked list or an even an array of pthread_key_t pointers, and use the int "key" as the index into that list. If I understand correctly this should be basically harmless since the same key (and hence key -> native-key mapping) can be shared across threads.
|msg273453 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-23 14:28|
(Of course, maintaining such a list might take some care, but only when creating and deleting keys--it wouldn't add any overhead to using them to get/set values.)
|msg273460 - (view)||Author: R. David Murray (r.david.murray) *||Date: 2016-08-23 14:51|
I'd say that sounds reasonable, but most likely it will only be someone working with one of the impacted platforms who will have the motivation to come up with a patch. Especially since neither of the impacted platforms is current formally supported (meaning, we don't yet have anyone on the core team working with those platforms). We have, it seems, arrived at the time that MvL foresaw.... :)
|msg273464 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-23 15:05|
The good news about this (in the pthread case) is that it does not need to be seen as some workaround for unusual platforms, but rather making the existing code more POSIX-compliant (and hence correct). The Win32 side I'm a little less worried about because the TLS key is documented to be a DWORD, and so the existing casting to int should still work fine most of the time. However, the docs for TlsAlloc() (https://msdn.microsoft.com/en-us/library/windows/desktop/ms686801(v=vs.85).aspx) do state: > The value of the TLS index should be treated as an opaque value; do not assume that it is an index into a zero-based array. which again makes #22206 suspect :(
|msg273480 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-23 16:29|
FWIW I've created an initial patch for this. Seems to work fine, but it's a bit of a mess and I have a few small implementation concerns. I'll try to get it cleaned up sometime in the next few days and I'll post it.
|msg273916 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-30 13:53|
Here's a first stab at a patch for this. A linked list is maintained which maps pthread_key_t instances to an int "key_id" that is used for the PyThread API. Each function needs to map a given key_id to the actual pthread_key_t. This adds a little bit of overhead of course, but in typical usage I don't think there are many entries in this list to loop over. This also reverts the change from #22206 which is no longer relevant. No synchronization is provided for PyThread_create_key and PyThread_delete_key, but in typical usage that would be the user's responsibility anyways. Otherwise every PyThread_*_key function would have to have a mutex around it.
|msg273919 - (view)||Author: STINNER Victor (haypo) *||Date: 2016-08-30 15:10|
-1 for your change issue25658-1.patch. It adds a O(n) slowdown to PyThread_set_key_value() whereas the performance of this function matters. Moreover, the code currently works fine on Linux, I fail to see why we should make Python slower to support a new platform. The PEP 11 explains the criteria to support a new platform in Python. Before we start to officially support CloudABI, you should start to work on a fork. When you will get enough users and have a better view of all requires changes, you can come back with a full plan to support officially this platform.
|msg274030 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-31 15:29|
Hi, First of all, I should be clear this is not just about CloudABI. In fact I don't even know what CloudABI is. I'm personally working toward getting a working/stable buildbot for Cygwin, so that I can move Python toward supporting it again. Cygwin has not been officially unsupported, but support for it is broken due to lack of a buildbot and lack of a maintainer, both of which I'm willing to offer. I can't get a stable buildbot though until some issues with the build are resolved, and this is a major blocker. If you prefer I can also work on a more formal plan to clarify Python's support for Cygwin which is currently in limbo. Not saying a solution to this needs to be merged ASAP as I can work on a branch, but in the meantime it's nice to have an initial fix for the issue so that it can be worked around--I'm sure other platforms that have this issue, such as the CloudABI folks, will appreciate that as well. Second of all, the point must be made that I'm not suggesting this be changed just in order to support some specific platform. The fact remains that the existing code is misusing the pthread API and is fragile to begin with. "It works on Linux" is not an excuse--not only does it suggest a bias, but the fact that it even works on Linux is an accident of implementation. The implementation of the pthread_key_t type could change tomorrow, and it would be Python's fault if that breaks things (not that I think that's at all likely to happen, though I'd be less surprised if OSX suddenly changed :) As for the performance I agree that PyThread_(get|set)_key_value should be fast. This may be O(n) but how big, typically, is n? In a normal run of the Python interpreter n=1, and the autoTLSkey is always the first in the list. It is only larger if some third-party code is using the PyThread_ API for TLS (and in most typical uses this will only be a few keys at the most, though I couldn't even find any examples in the wild where third-party code is using these functions). n could also grow on forks, or manual Py_Finialize/Py_Initialize. This patch didn't implement PyThread_ReInitTLS but it could, in which case it would reset next_key_id to 0 for each child process. Likewise, PyThread_ReInitTLS could also be called from Py_Initialize, which should be harmless. So TL;DR I'm not really convinced by the performance argument. But even if that were an issue other implementations are possible; this was just among the simplest and least wasteful.
|msg274035 - (view)||Author: Ed Schouten (EdSchouten) *||Date: 2016-08-31 15:46|
I think that PEP 11 also doesn't rule out changes in this area. For example, consider the paragraph starting with the sentence "This policy does not disqualify supporting other platforms indirectly. [...]" Attached is a patch that accomplishes the same with a constant running time for operations on keys by using an array. Allocation of new keys runs in expected constant time by using a randomised allocation pattern. If we're not big fans of using randomised access, changing this to a linear scan would already be an improvement: keys are only allocated infrequently, but hopefully accessed quite often.
|msg274040 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-08-31 17:08|
Ah, I wasn't thinking clearly toward the bottom of my last message. I see now that after a fork, _PyGILState_Reinit calls PyThread_delete_key followed by a new PyThread_create_key--in the current version of my patch that would result in putting the autoTLSkey at the end of the linked list, which is no good. That could be worked around, but... Ed's version looks good to me. I had the same idea as an alternative, though was a little concerned with the possibility that the array could grow too large. But as I wrote in my last message that would be an extreme case. And regardless his version will at least maintain constant time, so +1.
|msg280511 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2016-11-10 15:37|
Hi, I came from #28656. I have read past discussions, And I've understood the pthread_key_t has possible of either integer or pointer. So I think there is a simple solution that replaces key type to intptr_t. Thus I wrote a patch, but it maybe need configure check for intptr_t if CPytyhon still be supporting for before C99 compilers. I confirmed patch on Cygwin x86. Would you be able to confirm for other platforms?
|msg280512 - (view)||Author: Ed Schouten (EdSchouten) *||Date: 2016-11-10 15:55|
It can also be a structure or a union.
|msg280525 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2016-11-10 16:49|
Umm, API seems a design that is passing into function by integer or pointer because the users don't need type detail. I think the implementation of complex type is not realistic. Actually, CouldABI and Cygwin are used pointer, and type detail is hided.
|msg280526 - (view)||Author: Ed Schouten (EdSchouten) *||Date: 2016-11-10 16:56|
CloudABI uses a structure type, which is exactly why I filed this bug report. Instead of speculating about what kind of type existing implementations use, please just focus on the specification to which we're trying to stick: POSIX. http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html "All of the types shall be defined as arithmetic types of an appropriate length, with the following exceptions: [...] pthread_key_t [...]"
|msg280527 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2016-11-10 17:14|
On, I got complex type in cloudABI. I see my patch doesn't solve it. https://github.com/NuxiNL/cloudlibc/blob/master/src/include/sys/types.h#L94 https://github.com/NuxiNL/cloudlibc/blob/master/src/include/_/types.h#L209
|msg281227 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2016-11-19 19:28|
I wrote a patch to avoid compile error for platforms that pthread_key_t is not integer. This patch changes to turn off Py_HAVE_NATIVE_TLS if pthread_key_t is not integer. Hence the platforms use TLS functions implemented by CPython self. And, I would propose new thread local storage API based on C11 thread and current TLS functions move to deprecated. C11 tss_t doesn't require defined as integer . Therefore I think new API should use tss_t, not hide into integer.  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf (page 394) I'm thinking of new interfaces. For example, declaration in Include/pythread.h /* Specialise to each platforms using #if directive */ typedef /* TLS key types or C11 tss_t */ Py_tss_t; /* Based on C11 threads.h, but destructor doesn't support. the delete value function is maintained for the implementation by CPython self. */ PyAPI_FUNC(int) PyThread_tss_create(Py_tss_t *); PyAPI_FUNC(void) PyThread_tss_delete(Py_tss_t); PyAPI_FUNC(void *) PyThread_tss_get(Py_tss_t); PyAPI_FUNC(int) PyThread_tss_set(Py_tss_t, void *); PyAPI_FUNC(void) PyThread_tss_delete_value(Py_tss_t);
|msg281990 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-11-29 14:08|
To me, Masayuki's patch is an acceptable work-around for the time being. I don't *like* it because the fact remains that native TLS can work on these platforms and falling back on Python's slower implementation isn't necessary. That said, the previous patch attempts also add additional overhead that shouldn't be necessary. I agree the best thing to do would be to change this API, but that is a bigger issue I guess...
|msg282024 - (view)||Author: Ed Schouten (EdSchouten) *||Date: 2016-11-29 17:48|
Looks good to me!
|msg282033 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2016-11-29 19:29|
Elik, Ed, I have overlooked tracemalloc module raises deadlock if apply the patch. I found out a source comment on Modules/_tracemalloc.c:161 /* If your OS does not provide native thread local storage, you can implement it manually using a lock. Functions of thread.c cannot be used because they use PyMem_RawMalloc() which leads to a reentrant call. */ #if !(defined(_POSIX_THREADS) || defined(NT_THREADS)) # error "need native thread local storage (TLS)" #endif Py_HAVE_NATIVE_TLS is used only on thread source code. Therefore C Compiler couldn't report error about tracemalloc. I'm sorry that I didn't check test. Currently I'm trying to implement new API based on msg281227.
|msg282404 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2016-12-05 09:52|
I wrote a patch based on msg281227's idea. I confirmed to pass related tests (test_capi, test_threading and test_tracemalloc) on Cygwin x86 and Ubuntu 16.04 x86. This patch adds to change CPython about: 1. Avoid compile error on Currently TLS API. With POSIX threads, configure script checks pthread_key_t type. And if the type is not integer, TLS API is implemented empty (do nothing or return fail). 2. Implement New TSS API based on C11 threads. Thread key type is changed wrapped native TLS key type by typedef instead of integer, and the key parameter on key create function is changed to calling by pointer. Key create function doesn't support destructor because users have need to manage memory. And this patch changes Currently TLS API to call New TSS API. Or does it need to keep implementation? 3. Move to TSS API. Py_DEPRECATED(3.7) is added to TLS API declaration, and Modules/_tracemalloc.c and Python/pystate.c are modified to use TSS API instead of TLS API.
|msg282410 - (view)||Author: Erik Bray (erik.bray) *||Date: 2016-12-05 10:22|
I'm still pretty happy with the previous patch, personally, since I don't need the tracemalloc module. But it seems like that should be fixed (or if nothing else that code in _tracemalloc.c should check Py_HAVE_NATIVE_TLS too). I like the idea of the new PyThread_tss_ API. At first I wasn't sure because I thought you implied that it would use tss_t and related APIs from C11 which was going to be a non-starter (as CPython has only just barely started using *some* features from C99, per the recent update to PEP 7). But I see in your patch that the naming is only inspired by C11 (and could be consistent with it if CPython ever moves toward C11 support). I imagine this will likely require a PEP though? I would happy to help draft one.
|msg285039 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2017-01-09 12:46|
After Erik posted PEP 539 draft, we've discussed features of new API on the Python-ideas [*]. As the result of discussion, features of new API have been changed below points. Thus, I updated patch based on the result. 1. API uses opaque type because to cover the key details. 2. The key has the state that is whether initialized, and a function is added to check the state. 3. When key creation and deletion, API silently skips function in unnecessary case (i.e. valid key never be overwritten). 4. The test is added to check the key state that after calling API. [*] https://mail.python.org/pipermail/python-ideas/2016-December/043983.html
|msg285328 - (view)||Author: Erik Bray (erik.bray) *||Date: 2017-01-12 14:37|
Thanks Masayuki for the updated patch, and especially for the additional test cases. Looking at the patch, it occurs to me that this solution seems to be the minimal needed to fix the issue that we were originally trying to fix, without adding too much additional complexity or new semantics to how TLS keys are used in the interpreter. In other words, this preserves the existing usage with minimal changes except to better support opaque key types. So I think it's a point in favor of this change that's managed to remain focused. I'll work on updating the PEP draft to reflect the additions.
|msg285478 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2017-01-14 12:43|
I commented at the Rietveld, since I think that patch needs a bit more modification. * rename PyThread_ReInitTLS * update comments and messages that have explained CPython TLS API
|msg286012 - (view)||Author: Masayuki Yamamoto (masamoto) *||Date: 2017-01-22 15:11|
Above said, I updated minor changes to the version 2 patch. Several codes have kept the words "thread local" and "TLS" because they have pointed programming method or other meanings, not CPython TLS API itself. (e.g. _decimal module)
|msg290035 - (view)||Author: STINNER Victor (haypo) *||Date: 2017-03-23 10:13|
Hi people working on the new TLS API: I would like your opinion on a related API, issue #29881: Add a new private API for "static C variables" (_PyStaticVar) to clear them at exit !
|2017-03-23 10:13:07||haypo||set||messages: + msg290035|
messages: + msg286012
|2017-01-14 12:43:36||masamoto||set||messages: + msg285478|
|2017-01-12 14:37:42||erik.bray||set||messages: + msg285328|
messages: + msg285039
|2016-12-05 10:22:59||erik.bray||set||messages: + msg282410|
messages: + msg282404
|2016-11-29 19:29:13||masamoto||set||messages: + msg282033|
|2016-11-29 17:48:07||EdSchouten||set||messages: + msg282024|
|2016-11-29 14:08:56||erik.bray||set||messages: + msg281990|
messages: + msg281227
|2016-11-10 17:14:59||masamoto||set||messages: + msg280527|
|2016-11-10 17:05:25||serhiy.storchaka||link||issue28656 superseder|
|2016-11-10 16:56:58||EdSchouten||set||messages: + msg280526|
|2016-11-10 16:49:08||masamoto||set||messages: + msg280525|
|2016-11-10 15:55:16||EdSchouten||set||messages: + msg280512|
versions: + Python 3.7
nosy: + masamoto
messages: + msg280511
type: compile error
|2016-08-31 17:08:10||erik.bray||set||messages: + msg274040|
messages: + msg274035
|2016-08-31 15:29:07||erik.bray||set||messages: + msg274030|
|2016-08-30 15:10:36||haypo||set||messages: + msg273919|
|2016-08-30 13:54:04||erik.bray||set||stage: patch review|
keywords: + patch
|2016-08-30 13:53:18||erik.bray||set||messages: + msg273916|
|2016-08-23 16:31:34||r.david.murray||set||hgrepos: - hgrepo352|
|2016-08-23 16:29:35||erik.bray||set||messages: + msg273480|
|2016-08-23 15:05:56||erik.bray||set||messages: + msg273464|
+ Python 3.6, - Python 3.5|
nosy: + r.david.murray
messages: + msg273460
hgrepos: + hgrepo352
|2016-08-23 14:28:37||erik.bray||set||messages: + msg273453|
|2016-08-23 14:13:44||erik.bray||set||messages: + msg273451|
messages: + msg273443
messages: + msg273441