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

Use WinAPI concurrency primitives on Windows ports (remove winpthreads) #58

Closed
wants to merge 16 commits into from

Conversation

MisterDA
Copy link
Owner

@MisterDA MisterDA commented May 27, 2024

The goal is to remove winpthreads (a library emulating pthreads on top of old Windows APIs), and use modern Windows APIs in both the MSVC and MinGW-w64 ports.

It would be great if this PR could be considered for inclusion in 5.3: although we were already using the system winpthreads for the MinGW-w64 port, we introduced it as a submodule during the 5.3 development cycle to support the MSVC port. If we could remove it now, end users won't ever (unknowingly) depend on it.

One commit removes winpthreads (breaks OCaml build on Windows) and another implements the caml_plat_* and st_* functions required to abstract over pthreads and Windows concurrency APIs.

Note that Windows Mutex Objects cannot be used with condition variables. The right pairing consists of Slim Reader/Writer Locks (SRW locks) and condition variables. SRW locks support shared locking (only readers) and exclusive locking (one writer). I've chosen to use exclusive locking in all cases. Windows condition variables have the problem that they cannot be statically initialized.

Both pthreads and WinAPI are quite similar, except for a few points that I've noted:

  1. The type of a function that starts a thread. They return a void * pointer on pthreads, and an unsigned int on Windows, but it seems not to be a concern for us.

  2. We use pthread ERRORCHECK mutexes. POSIX states:

    The pthread_mutex_lock() function shall fail if:
    [EDEADLK]
    The mutex type is PTHREAD_MUTEX_ERRORCHECK and the current thread already owns the mutex.

    The Windows documentation states:

    Exclusive mode SRW locks cannot be acquired recursively. If a thread tries to acquire a lock that it already holds, that attempt will […] deadlock (for AcquireSRWLockExclusive).

    Deadlocks caused by recursive attempts to lock cannot be detected on Windows. We cannot either detect double unlocks, or attempts to unlock a lock owned by another thread. This would revert the Stdlib.Mutex module guarantees to those of before OCaml 4.12 on Windows only.

    I've thus chosen to implement so-called ERRORCHECK mutexes with a pair of a SRWLock, and the thread id of the owner, accessed atomically. The value 0 is used when the thread is not owned, and is a valid sentinel value.

  3. A Windows thread is "detached" if all handles referring to the thread have been closed: it cannot be joined (waited on in Windows terminology) anymore but is not ended. pthread has a thread attribute to control the detached state.

@MisterDA MisterDA force-pushed the winpthreadsectomy branch 9 times, most recently from 04f42af to c2319cc Compare May 30, 2024 14:33
@MisterDA MisterDA changed the title Winpthreadsectomy Use WinAPI concurrency primitives on Windows ports (remove winpthreads) May 30, 2024
@MisterDA MisterDA force-pushed the winpthreadsectomy branch 4 times, most recently from 8176695 to 59187e1 Compare May 30, 2024 16:42
@MisterDA MisterDA force-pushed the winpthreadsectomy branch 8 times, most recently from deedfb5 to eb4a3c1 Compare June 11, 2024 12:18
@MisterDA MisterDA force-pushed the winpthreadsectomy branch 8 times, most recently from 1f0e3e5 to 1a40d58 Compare July 25, 2024 10:13
@MisterDA MisterDA force-pushed the winpthreadsectomy branch 3 times, most recently from bb6aec9 to 792265e Compare August 26, 2024 08:37
Copy link
Collaborator

@shym shym left a comment

Choose a reason for hiding this comment

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

This looks good to me, even if this scraps out much of my code :-)

I just noted a couple of small suggestions:

  • 04b344f Implement concurrency primitives using WinAPI
    Maybe add in the commit message explanations about:

    • how code is reshuffled (what moves into unix.c, etc.)
    • detaching threads on Windows; also add a small comment in the _detach function about that (the handle cannot be duplicated, can it?)

    Is there a reason not to typedef HANDLE caml_plat_thread instead of casting it on occurrences?

    Could caml_plat_thread_create have the same signature on Windows (maybe asserting that only the supported attributes are used (only NULL, maybe? or also detached?))

otherlibs/systhreads/st_stubs.c Outdated Show resolved Hide resolved
@MisterDA MisterDA force-pushed the winpthreadsectomy branch 5 times, most recently from 396428c to 324a583 Compare August 27, 2024 14:05
Copy link
Collaborator

@shym shym left a comment

Choose a reason for hiding this comment

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

I’ve reviewed the range-diff with the previous version I had reviewed. This looks good!

runtime/win32.c Outdated Show resolved Hide resolved
runtime/win32.c Outdated Show resolved Hide resolved
gasche and others added 4 commits September 2, 2024 21:17
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Substitute and remove the copyright header.

    sed -i \
      -e '/#include "st_pthreads.h"/{r otherlibs/systhreads/st_pthreads.h' \
      -e 'd}' \
      otherlibs/systhreads/st_win32.h

Done to ease the diff when switching to WinAPI primitives.
@MisterDA MisterDA force-pushed the winpthreadsectomy branch 3 times, most recently from 169dbf5 to 2fb4afe Compare September 3, 2024 09:33
MisterDA and others added 3 commits September 3, 2024 11:36
Don't include pthread.h on Windows to avoid using pthreads emulation
libraries (such as winpthreads) by mistake.

Functions from platform.c that used pthreads are moved to unix.c,
their Windows counterparts are implemented in win32.c. Introduce a new
sync_win32.h header, counterpart of sync_unix.h. Introduce new
caml_plat_thread and caml_plat_thread_attr to abstract over the
system's thread id/handle type and thread attributes.

Use pairs of SRWLOCK and CONDITION_VARIABLES to replace pthreads
mutexes and condition variables.

POSIX states:

> The pthread_mutex_lock() function shall fail if:
> [EDEADLK]
>   The mutex type is PTHREAD_MUTEX_ERRORCHECK and the current thread
>   already owns the mutex.

The Windows documentation states:

> Exclusive mode SRW locks cannot be acquired recursively. If a thread
> tries to acquire a lock that it already holds, that attempt will
> [...] deadlock (for AcquireSRWLockExclusive).

Deadlocks caused by recursive attemps to lock cannot be detected on
Windows.
See also PR 10220 and issue 8857.
SetLastError is called when accessing the TLS value This_thread.

(cherry picked from commit 9384357)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants