bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API #1362

ma8ma · 2017-04-30T07:56:14Z

~~Do not merge until approved the PEP final!~~
Update (2017-09-09): PEP 539 has been accepted.

Thread Specific Storage (TSS) API

Implement PEP 539 for Thread Specific Stroage (TSS) API: it is a new Thread Local Storage (TLS) API to CPython which would supersede use of the existing TLS API within the CPython interpreter, while deprecating the existing API.

Highlights of changes

Add Thread Specific Storage (TSS) API
Add test for transitions of thread key state
Mark deprecation to Thread Local Storage (TLS) API
Replace codes that used TLS API with TSS API
Add the TSS API in the Windows stable ABI (bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API #1362 (comment))
~~[TEMP] Add an example function to xxlimited module~~

https://bugs.python.org/issue25658

mention-bot · 2017-04-30T07:56:16Z

@ma8ma, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mhammond, @serhiy-storchaka and @loewis to be potential reviewers.

See PEP 539 for details. highlights of changes: - Add Thread Specific Storage (TSS) API - Add test for transitions of the thread key state. - Mark deprecation to Thread Local Storage (TLS) API - Replace codes that used TLS API with TSS API

AraHaan · 2017-04-30T08:35:07Z

Uh are you rebasing? I made that mistake before. The latest dev guide says to not rebase but include all commits which would then be merged as a single commit.

ma8ma · 2017-04-30T08:56:19Z

@AraHaan Yes, PR is single commit. I confirmed AppVeyor build failed, and retried push in a minute. But it has failed again, I need help for Windows platform...

AraHaan · 2017-04-30T10:16:52Z

It seems to be something with the solution or project settings or even AppVeyor's setting. Will investigate more when I can use the internet on my Inspiron 1545 that has Windows 7 Ultimate SP1 x64 and VS2015 installed. It seems that in the project settings is set to use the Windows sdk for 8.1 instead of for 10. Also when targeting for 10.0 I recommend staticallylinking the ucrt for the versions of Windows that can't install it (Vista). Also it allows flexibility and usability on fresh installs of 7 and Vista do that is another reason why I recommend it.

ncoghlan

This looks excellent to me as a solution for the standard ABI, but I realised in reviewing it that the stable ABI poses a potential problem: we don't want to expose the Py_tss_t definition in that case, but making the API usable with an opaque type definition would require a few changes (details inline).

ncoghlan · 2017-04-30T16:06:37Z

Include/pythread.h

+
+/* Py_tss_t is opaque type and you *must not* directly read and write.
+   When you'd check whether the key is created, use PyThread_tss_is_created.
+*/


This comment should explain that the type layout is public (despite being nominally opaque) to allow for static declarations in API clients.

It should be made fully opaque when Py_LIMITED_API is defined, though: https://docs.python.org/3/c-api/stable.html

ncoghlan · 2017-04-30T16:20:26Z

Include/pythread.h

+PyThread_tss_is_created(Py_tss_t key)
+{
+    return key._is_initialized;
+}


To allow for use in the Py_LIMITED_API case, the public APIs that currently accept Py_tss_t by value will need to accept it as a pointer instead.

The alternative would be to make the Py_tss_t struct layout part of the stable ABI, but I'm reluctant to do that due to the NATIVE_TLS_KEY_T field: it would keep us from switching from int to a different native type for platforms that don't currently have a native implementation, but gain one in the future.

Following that chain of thought further, I guess fully supporting Py_LIMITED_API would require an extra pair of memory management functions (PyThread_tss_alloc and PyThread_tss_free), as with the struct definition hidden, API clients won't be able to use static variables.

I'm not super-familiar with the constraints regarding Py_LIMITED_API. Addressing this will require an update to the PEP as well.

I wouldn't mind if the functions in question were changed to take a pointer argument instead of passing the struct by value just for consistency's sake if nothing else.

Okay, I start work to modify to full Py_LIMITED_API on the weekend.

With a PyThread_tss_alloc, would the need for Py_tss_NEEDS_INIT to be public go away? After all, if one used PyThread_tss_alloc to allocate memory for a struct, what else would they even initialize it with? It would remove an extra step to just do that as part of PyThread_tss_alloc and remove that implementation detail.

It should just be clear that just because a TSS key has been allocated doesn't mean it's been created. Also, PyThread_tss_is_created is still available to test whether or not a key has been created.

static storage allocation would still be supported for the normal (non-limited API) case, so the public Py_tss_NEEDS_INIT initializer would still be needed.

ncoghlan · 2017-04-30T16:29:27Z

Python/thread.c

+
+/* Thread Local Storage (TLS) API, DEPRECATED since Python 3.7
+
+   In the own implementation, TLS API has been changed to call TSS API.


For the second sentence here, I'd suggest "While the native thread-local storage implementations have been retained, this fallback implementation is now just a compatibility wrapper around the TSS API."

ncoghlan · 2017-04-30T16:36:11Z

Python/thread_foobar.h

+
+void
+PyThread_tss_delete_value(Py_tss_t key)
+{


Does PEP 7 express a preference regarding empty function definitions? If it doesn't we should at least make sure the TLS/TSS code is self-consistent (one of the later changes has the opening and closing brace on the same line).

Not that I can tell, in which case the most consistent style would be that demonstrated here.

Sure, I'm going to make formatting even. By the way, would I create an issue for removing Python/thread_foobar.h? Although I'm not sure history of thread_foobar.h, it seems to me that is dead code at present.

@ma8ma Aye, a separate issue and python-dev thread for that sounds sensible to me. The question to the mailing list can just be something simple like "This looks like dead code and we want to delete it, does anyone have a reason we should keep it?"

I had assumed it was a dummy implementation for testing / demonstration purposes. It might be an easier way to understand the requirements of the threading API without being bogged down with the details of specific implementations. That said, I don't know how useful that is in practice. I don't think I ever looked at it, opting instead to look at the pthread implementation since pthreads are already familiar...

Well, in fact, I think it makes the loading to maintainers. I posted to python-dev.

ncoghlan · 2017-05-03T05:16:43Z

Include/pythread.h

+*/
+
+#if defined(_POSIX_THREADS)
+#   define NATIVE_TLS_KEY_T     pthread_key_t


For consistency, this should be NATIVE_TSS_KEY_T

ma8ma · 2017-05-07T13:54:13Z

In Windows, the module-definition (.def) file for limited API (python3.dll) doesn't have the TSS API (also the TLS API), therefore, xxlimited module build occurs link error. ~~Would I append TSS API to python3.def to update stable ABI?~~

Edit (2017-05-13): Add WIP commit for Windows ABI
Edit (2017-08-31): ~~As far as someone doesn't show a necessary reason, I'd revert WIP commit that adding TSS API into Windows stable ABI. (i.e. back to same support level as the existing TLS API)~~
Update (2017-09-02): ~~Revert a temporary commit which supports TSS API in Windows stable ABI, because the existing TLS API is not provided in Windows stable ABI. TSS API support goes to maintain the status quo.~~
Update (2017-09-18): Add the commit again (see #1362 (comment)).

PCbuild description

Resolve conflicts: fdaeea6 bpo-30279: Remove unused Python/thread_foobar.h (python#1473)

Resolve conflcts: ab4413a bpo-30039: Don't run signal handlers while resuming a yield from stack (python#1081)

Resolve conflicts: 346cbd3 bpo-16500: Allow registering at-fork handlers (python#1715)

The code has been moved from Modules/signalmodule.c. (346cbd3)

Resolve conflicts: f7ecfac Doc nits for bpo-16500 (python#1841)

Resolve conflicts: 3b5cf85 bpo-30524: Write unit tests for FASTCALL (python#2022)

python#1362 (comment)

ma8ma · 2017-09-18T07:42:44Z

@ncoghlan Thank you for the explanation. I don't think there is any problem, so I add the TSS API in the Windows stable ABI.

- overview - the existing TLS API (without descripiton) - the new TSS API - type and macro - dynamic allocation - methods

Several comments is no longer necessary by API document.

New order is along API document.

ma8ma · 2017-10-06T09:03:25Z

I updated this PR to add API document. Sources of the API description are:

PEP 539
comments on the code (5073d66)

Documents rendered by github:

ncoghlan

This is looking very good to me. I'm going to go through and make some edits to the docs updates, and add in the required limited API version check, but after that I think this should be ready to merge.

bedevere-bot · 2017-10-06T10:05:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

ncoghlan · 2017-10-06T10:09:24Z

Oops, the required macro version check is there, it just wasn't visible in the subset of changes I was looking at - so I'll just make the docs tweaks, and then this will be ready to merge.

embray · 2017-10-06T10:16:43Z

Great! I was just about to say, since --without-threads was removed it's impossible to compile on Cygwin without this now. Is there anything else I can do to help or is this otherwise ready to go?

ncoghlan · 2017-10-06T10:27:13Z

@embray I think it's good to go once the latest CI run finishes, but another set of eyes looking over the changes wouldn't hurt (if it's just docs comments though, then it would probably make sense to defer those to a new PR so we can restore Cygwin compatibility in the meantime)

ncoghlan · 2017-10-06T10:42:31Z

And merged - thank you @ma8ma for the PR!

ma8ma · 2017-10-06T10:56:22Z

@embray @ncoghlan Thank you for your advice too! 😄

pradyunsg · 2018-07-20T03:10:33Z

you will be put in the comfy chair!

This link doesn't seem to be working for me. I'm also watching other videos on YouTube currently so this seems to be this specific URL.

Screenshot -- click to view

qwhz

the-knights-who-say-ni added the CLA signed label Apr 30, 2017

ma8ma force-pushed the pep539-tss-api branch from c3f38df to 4ebc65b Compare April 30, 2017 08:24

ma8ma force-pushed the pep539-tss-api branch from 4ebc65b to 23a6e59 Compare April 30, 2017 08:31

ncoghlan requested changes Apr 30, 2017

View reviewed changes

brettcannon added the type-feature A feature request or enhancement label May 1, 2017

ncoghlan reviewed May 3, 2017

View reviewed changes

ma8ma force-pushed the pep539-tss-api branch 3 times, most recently from 38138e2 to dff0a13 Compare May 7, 2017 13:16

ma8ma force-pushed the pep539-tss-api branch 2 times, most recently from 1c98d1b to 5b9b417 Compare May 13, 2017 12:37

ma8ma force-pushed the pep539-tss-api branch 4 times, most recently from 56d660b to 23a6e59 Compare June 13, 2017 14:05

ma8ma added 9 commits June 13, 2017 23:10

Merge commit 'master' into pep539-tss-api

8e8aa5d

Resolve conflicts: fdaeea6 bpo-30279: Remove unused Python/thread_foobar.h (python#1473)

Merge commit 'master' into pep539-tss-api

178baec

Resolve conflcts: ab4413a bpo-30039: Don't run signal handlers while resuming a yield from stack (python#1081)

Merge commit 'master' into pep539-tss-api

08f1fa8

Resolve conflicts: 346cbd3 bpo-16500: Allow registering at-fork handlers (python#1715)

bpo-25658: Rename ReInitTLS function in Modules/posixmodule.c

17b1be7

The code has been moved from Modules/signalmodule.c. (346cbd3)

Merge commit 'master' into pep539-tss-api

027dc1d

Resolve conflicts: f7ecfac Doc nits for bpo-16500 (python#1841)

Merge commit 'master' into pep539-tss-api

e49207b

Resolve conflicts: 3b5cf85 bpo-30524: Write unit tests for FASTCALL (python#2022)

bpo-25658: Modify comment about wrapper function

2f57cc0

python#1362 (comment)

bpo-25658: Rename native TSS key type macro

f827c45

python#1362 (comment)

bpo-25658: Format empty function definitions

b0e5ce1

python#1362 (comment)

bpo-25658: Add the TSS API to Windows stable ABI

a0d742a

ma8ma added 6 commits October 6, 2017 16:54

Merge branch 'master' into pep539-tss-api

2796a06

bpo-25658: remove API description on the code

5073d66

bpo-25658: add API document

c9f7536

- overview - the existing TLS API (without descripiton) - the new TSS API - type and macro - dynamic allocation - methods

bpo-25658: modify whatsnew document

a9a9df6

bpo-25658: modify header comments

60828d8

Several comments is no longer necessary by API document.

bpo-25658: update order of the declaration

f1717b4

New order is along API document.

ncoghlan requested changes Oct 6, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Oct 6, 2017

Adjust some details of documentation wording

80e7684

ncoghlan approved these changes Oct 6, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 6, 2017

ncoghlan merged commit 731e189 into python:master Oct 6, 2017

bedevere-bot removed the awaiting merge label Oct 6, 2017

ma8ma deleted the pep539-tss-api branch October 8, 2017 03:57

ma8ma mentioned this pull request Oct 20, 2017

bpo-31828: make Py_tss_NEEDS_INIT usable in non-static initialisers #4060

Closed

embray mentioned this pull request Nov 6, 2017

[3.6] bpo-31878: Fix _socket module compilation on Cygwin. (GH-4137) #4145

Merged

shaorongchen88 reviewed Jul 18, 2018

View reviewed changes

qwhz reviewed Feb 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API #1362

bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API #1362

ma8ma commented Apr 30, 2017 •

edited

Loading

mention-bot commented Apr 30, 2017

AraHaan commented Apr 30, 2017

ma8ma commented Apr 30, 2017

AraHaan commented Apr 30, 2017 •

edited

Loading

ncoghlan left a comment

ncoghlan Apr 30, 2017

ncoghlan Apr 30, 2017

embray May 2, 2017

ma8ma May 3, 2017

embray May 3, 2017

ncoghlan May 3, 2017

ncoghlan Apr 30, 2017

ncoghlan Apr 30, 2017

embray May 2, 2017

ma8ma May 3, 2017

ncoghlan May 3, 2017

embray May 3, 2017

ma8ma May 3, 2017

ncoghlan May 3, 2017

ma8ma commented May 7, 2017 •

edited

Loading

ma8ma commented Sep 18, 2017

ma8ma commented Oct 6, 2017

ncoghlan left a comment

bedevere-bot commented Oct 6, 2017

ncoghlan commented Oct 6, 2017

embray commented Oct 6, 2017

ncoghlan commented Oct 6, 2017

ncoghlan commented Oct 6, 2017

ma8ma commented Oct 6, 2017

pradyunsg commented Jul 20, 2018

qwhz left a comment


		/* Thread Local Storage (TLS) API, DEPRECATED since Python 3.7

		In the own implementation, TLS API has been changed to call TSS API.

bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API #1362

bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API #1362

Conversation

ma8ma commented Apr 30, 2017 • edited Loading

Thread Specific Storage (TSS) API

Highlights of changes

mention-bot commented Apr 30, 2017

AraHaan commented Apr 30, 2017

ma8ma commented Apr 30, 2017

AraHaan commented Apr 30, 2017 • edited Loading

ncoghlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma8ma commented May 7, 2017 • edited Loading

ma8ma commented Sep 18, 2017

ma8ma commented Oct 6, 2017

ncoghlan left a comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 6, 2017

ncoghlan commented Oct 6, 2017

embray commented Oct 6, 2017

ncoghlan commented Oct 6, 2017

ncoghlan commented Oct 6, 2017

ma8ma commented Oct 6, 2017

pradyunsg commented Jul 20, 2018

qwhz left a comment

Choose a reason for hiding this comment

ma8ma commented Apr 30, 2017 •

edited

Loading

AraHaan commented Apr 30, 2017 •

edited

Loading

ma8ma commented May 7, 2017 •

edited

Loading