◐ Shell
reader mode source ↗
Skip to content

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

Merged
ncoghlan merged 55 commits into
python:masterfrom
ma8ma:pep539-tss-api
Oct 6, 2017
Merged

bpo-25658: Implement PEP 539 for Thread Specific Storage (TSS) API#1362
ncoghlan merged 55 commits into
python:masterfrom
ma8ma:pep539-tss-api

Conversation

@ma8ma

@ma8ma ma8ma commented Apr 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

https://bugs.python.org/issue25658

@mention-bot

mention-bot commented Apr 30, 2017

Copy link
Copy Markdown

@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.

@ma8ma
Implement PEP 539 Thread Specific Storage (TSS) API …
23a6e59 
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

AraHaan commented Apr 30, 2017

Copy link
Copy Markdown
Contributor

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

ma8ma commented Apr 30, 2017

Copy link
Copy Markdown
Contributor Author

@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

AraHaan commented Apr 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
ncoghlan requested changes Apr 30, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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).

@brettcannon brettcannon added the type-feature A feature request or enhancement label May 1, 2017
@ma8ma ma8ma force-pushed the pep539-tss-api branch 3 times, most recently from 38138e2 to dff0a13 Compare May 7, 2017 13:16
@ma8ma

ma8ma commented May 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@ma8ma ma8ma force-pushed the pep539-tss-api branch 2 times, most recently from 1c98d1b to 5b9b417 Compare May 13, 2017 12:37
@ma8ma 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
@ma8ma
Merge commit 'master' into pep539-tss-api …
8e8aa5d 
Resolve conflicts:
fdaeea6 bpo-30279: Remove unused Python/thread_foobar.h (python#1473)
@ma8ma
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)
@ma8ma
Merge commit 'master' into pep539-tss-api …
08f1fa8 
Resolve conflicts:
346cbd3 bpo-16500: Allow registering at-fork handlers (python#1715)
@ma8ma
bpo-25658: Rename ReInitTLS function in Modules/posixmodule.c …
17b1be7 
The code has been moved from Modules/signalmodule.c. (346cbd3)
@ma8ma
Merge commit 'master' into pep539-tss-api …
027dc1d 
Resolve conflicts:
f7ecfac Doc nits for bpo-16500 (python#1841)
@ma8ma
Merge commit 'master' into pep539-tss-api …
e49207b 
Resolve conflicts:
3b5cf85 bpo-30524: Write unit tests for FASTCALL (python#2022)
@ma8ma
bpo-25658: Modify comment about wrapper function …
2f57cc0 
python#1362 (comment)
@ma8ma
bpo-25658: Rename native TSS key type macro …
f827c45 
python#1362 (comment)
@ma8ma
bpo-25658: Format empty function definitions …
b0e5ce1 
python#1362 (comment)
99 hidden items Load more…
@ma8ma

ma8ma commented Sep 18, 2017

Copy link
Copy Markdown
Contributor Author

@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.

ma8ma added 6 commits October 6, 2017 16:54
@ma8ma
Merge branch 'master' into pep539-tss-api
2796a06
@ma8ma
bpo-25658: remove API description on the code
5073d66
@ma8ma
bpo-25658: add API document …
c9f7536 
- overview
- the existing TLS API (without descripiton)
- the new TSS API
  - type and macro
  - dynamic allocation
  - methods
@ma8ma
bpo-25658: modify whatsnew document
a9a9df6
@ma8ma
bpo-25658: modify header comments …
60828d8 
Several comments is no longer necessary by API document.
@ma8ma
bpo-25658: update order of the declaration …
f1717b4 
New order is along API document.
@ma8ma

ma8ma commented Oct 6, 2017

Copy link
Copy Markdown
Contributor Author

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
ncoghlan requested changes Oct 6, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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

bedevere-bot commented Oct 6, 2017

Copy link
Copy Markdown

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

ncoghlan commented Oct 6, 2017

Copy link
Copy Markdown
Contributor

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

embray commented Oct 6, 2017

Copy link
Copy Markdown
Contributor

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

ncoghlan commented Oct 6, 2017

Copy link
Copy Markdown
Contributor

@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 ncoghlan merged commit 731e189 into python:master Oct 6, 2017
@ncoghlan

ncoghlan commented Oct 6, 2017

Copy link
Copy Markdown
Contributor

And merged - thank you @ma8ma for the PR!

@ma8ma

ma8ma commented Oct 6, 2017

Copy link
Copy Markdown
Contributor Author

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

@ma8ma ma8ma deleted the pep539-tss-api branch October 8, 2017 03:57
@ma8ma ma8ma mentioned this pull request Oct 20, 2017
Closed
@embray embray mentioned this pull request Nov 6, 2017
Merged
@pradyunsg

pradyunsg commented Jul 20, 2018

Copy link
Copy Markdown
Member

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 screen shot 2018-07-20 at 8 37 51 am

qwhz
qwhz reviewed Feb 15, 2020
View reviewed changes

@qwhz qwhz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hide comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba left review comments

@ncoghlan ncoghlan

+4 more reviewers

@embray embray embray left review comments

@shaorongchen88 shaorongchen88 shaorongchen88 left review comments

@qwhz qwhz

@eamanu eamanu eamanu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ncoghlan ncoghlan

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.