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

Jump to bottom

thread_nt.c update #47832

Closed
kristjanvalur mannequin opened this issue Aug 17, 2008 · 13 comments
Closed

thread_nt.c update #47832

kristjanvalur mannequin opened this issue Aug 17, 2008 · 13 comments
Assignees
@kristjanvalur
Labels
type-feature A feature request or enhancement

Comments

@kristjanvalur
Copy link
Mannequin

kristjanvalur mannequin commented Aug 17, 2008

BPO 3582
Nosy @loewis, @amauryfa, @kristjanvalur
Files
  • thread_nt.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/kristjanvalur'
closed_at = <Date 2009-01-12.22:06:41.591>
created_at = <Date 2008-08-17.21:00:17.805>
labels = ['type-feature']
title = 'thread_nt.c update'
updated_at = <Date 2009-01-19.10:31:50.732>
user = 'https://github.com/kristjanvalur'

    bugs.python.org fields:

    activity = <Date 2009-01-19.10:31:50.732>
actor = 'kristjan.jonsson'
assignee = 'kristjan.jonsson'
closed = True
closed_date = <Date 2009-01-12.22:06:41.591>
closer = 'loewis'
components = []
creation = <Date 2008-08-17.21:00:17.805>
creator = 'kristjan.jonsson'
dependencies = []
files = ['11141']
hgrepos = []
issue_num = 3582
keywords = ['patch']
message_count = 13.0
messages = ['71289', '79179', '79180', '79188', '79236', '79238', '79239', '79499', '79503', '79653', '79656', '79657', '80154']
nosy_count = 4.0
nosy_names = ['loewis', 'amaury.forgeotdarc', 'kristjan.jonsson', 'eckhardt']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue3582'
versions = ['Python 3.0']
    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Aug 17, 2008

    Here is a suggested update to thread_nt.c. It has two significant
    changes:

    1. it uses the new and safer _beginthreadex API to start a thread
    2. it implements native TLS functions on NT, which are certain to be as
      fast as possible.

    @kristjanvalur kristjanvalur mannequin added the type-feature A feature request or enhancement label Aug 17, 2008
    @amauryfa
    Copy link
    Member

    amauryfa commented Jan 5, 2009

    • Isn't CreateThread even better?

    • Using TLS functions is a good idea, beware though that the number of
      TLS slots cannot exceed 1088 on Windows. This should at least be documented.
      OTOH since there is only one call to PyThread_create_key in the codebase
      (for the GIL), and I found only one more through the Google Code Search,
      it's probably not worth the trouble.

    @eckhardt
    Copy link
    Mannequin

    eckhardt mannequin commented Jan 5, 2009

    No, CreateThread() is not a suitable replacement, because it lacks
    some initialisations of the C library, as explained in the MSDN:
    http://msdn.microsoft.com/en-us/library/ms682453(VS.85).aspx

    @amauryfa
    Copy link
    Member

    amauryfa commented Jan 5, 2009

    You are right.
    (My dream is someday to completely get rid of this msvcrt stuff. Only
    pure Win32 API)

    @eckhardt
    Copy link
    Mannequin

    eckhardt mannequin commented Jan 6, 2009

    I do have an issue with the patch's startup code. The prototype for
    the thread entry should be "DWORD WINAPI function(PVOID);". The
    important distinction is the WINAPI part, which resolves to __stdcall
    but doesn't have to. I know that some CE targets actually #define
    __cdecl __stdcall or vice versa, but using WINAPI always works. I'd
    then also change the comment to "...to adapt between our function
    signature and the one used by _createthreadex", as the internally used
    one doesn't mention __cdecl.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Jan 6, 2009

    No, the startup function for beginthreadex() must be __stdcall. From
    the docs:

    start_address
    Start address of a routine that begins execution of a new thread. For
    _beginthread, the calling convention is either __cdecl or __clrcall;
    for _beginthreadex, it is either __stdcall or __clrcall.

    (__clrcall is for managed code which doesn't apply).

    Do not confuse beginthreadex() with CreateThread even though they have
    similar arguments. The former is for use with programs using the crt.

    See http://msdn.microsoft.com/en-us/library/kdzttdcb.aspx for details.

    Amaury, Where did you find the value 1088 for maximum TLS slots? The
    documentation mentions the error condidion TLS_OUT_OF_INDEXES (which I
    should check for, I suppose), but nothing else.

    @eckhardt
    Copy link
    Mannequin

    eckhardt mannequin commented Jan 6, 2009

    You're right, Kristján, sorry for the noise. Shouldn't make claims
    before being fully awake....

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Jan 9, 2009

    Committed this as revision: 68455

    @kristjanvalur kristjanvalur mannequin closed this as completed Jan 9, 2009
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 9, 2009

    Windows NT (3.1, and a number of later versions) only support 64 TLS
    keys. Starting with Windows 2000, they added another page per thread for
    TLS, giving an addition 1024 TLS slots, for a total of 1088 TLS slots.
    FWIW, Win 9.x supported 80 TLS slots. See

    http://www.nynaeve.net/?p=181
    http://msdn.microsoft.com/en-us/library/ms686749.aspx
    http://bugs.python.org/file11141/thread_nt.patch

    TLS slots are typically considered a scarce resource, so that
    programming language implementations typically don't allow applications
    direct allocation of TLS slots. Instead, most runtimes I know of will
    allocate a single TLS slot for themselves, which then is filled with an
    array or a dictionary.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 12, 2009

    Please add a Misc/NEWS entry for this change.

    @loewis loewis mannequin reopened this Jan 12, 2009
    @loewis loewis mannequin assigned kristjanvalur Jan 12, 2009
    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Jan 12, 2009

    Note, see defect 4906 for fallout caused by this checkin.

    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Jan 12, 2009

    Misc/NEWS updated in revision: 68544

    @loewis loewis mannequin closed this as completed Jan 12, 2009
    @kristjanvalur
    Copy link
    Mannequin Author

    kristjanvalur mannequin commented Jan 19, 2009

    Note, this has been ported to py3k in http://svn.python.org/view?
    view=rev&rev=68543

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @kristjanvalur kristjanvalur

    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @amauryfa