CoreFoundation Windows Port #1783
Conversation
|
Note that this is not fully complete, I'm still working through the patches. But, I needed a way to track the patch set. Most of the changes are pretty small and very targeted at individual classes of issues, so this is going to look much worse than it really is. It is one of those death by a thousand paper cuts scenario. |
compnerd
force-pushed
the
windows
branch
15 times, most recently
from
18e7e0b to
c29e925
Compare
|
CC: @millenomi |
|
|
||
| #elif DEPLOYMENT_TARGET_LINUX || DEPLOYMENT_TARGET_FREEBSD |
There was a problem hiding this comment.
CFLock is not really supposed to be a full pthread_mutex, instead it should be a super-lightweight lock ala futex or os_unfair_lock etc
There was a problem hiding this comment.
Ah, okay. What about the other pthread_mutex_t instances? This is a problem since there is no pthread_t on Windows, so trying to figure out how to define this properly is interesting. The other thing that complicates this is that the types for recursive mutexes and mutexes are different, so you cannot just use pthread_mutex_t as a typedef.
| @@ -119,7 +119,7 @@ CONST_STRING_DECL(_kCFBundleCFMLoadAsBundleKey, "CFBundleCFMLoadAsBundle") | |||
| // Keys used by NSBundle for loaded Info plists. | |||
| CONST_STRING_DECL(_kCFBundlePrincipalClassKey, "NSPrincipalClass") | |||
|
|
|||
| static pthread_mutex_t CFBundleGlobalDataLock = PTHREAD_MUTEX_INITIALIZER; | |||
| static CFLock_t CFBundleGlobalDataLock = CFLockInit; | |||
There was a problem hiding this comment.
These are meant to be the pthread lock on Darwin. @phausler what do you think about switching them to the CFLock there too?
| #if DEPLOYMENT_TARGET_WINDOWS | ||
| EnterCriticalSection(&(rlm->_lock)); | ||
| #else | ||
| __CFLock(&(rlm->_lock)); |
There was a problem hiding this comment.
These ones too, I think need to remain a pthread lock on Darwin at least.
There was a problem hiding this comment.
The CFLock_t is always a pthread_t on Darwin. That's what I had changed Linux to, but seems that it was meant to be a lighter weight primitive like futex on Linux.
|
I realize that this work is mostly useful only when available in aggregate, however, I think that it would help reduce some of the cognitive overhead and management burden to pick out the patches that are ready to go and merge those in. Is that okay with you @parkera, @millenomi? |
|
Yes I think so. |
|
@phausler, @parkera - so, for the locking changes, what do you guys think of introducing a new type alias that we can use to differentiate between what needs to be a fully mutex ( |
|
As far as I know we never use an actually recursive lock - just the |
|
Really the two lock types are fair and unfair. pthread_mutex is fair, others are not. I am not convinced we really ever need fair locks even in Darwin but that is something we need to confirm to change. The unfair case for Darwin donates QoS via os_unfair_lock and Linux i don’t think really has QoS anyhow so a lightweight lock is fine (most Linuxes pthread_mutex is lighter than Darwin anyhow). So in the end the two concepts are heavy but fair lock and lightweight lock. |
|
@parkera, @phausler - there are definitely cases of recursive mutexes in the code base. Thats why I ended up using The problem is that the locking mechanism on Windows doesn't have posix locking, so we need a different set of locks. For lightweight mutexes that do not need to recursive, the Slim Reader/Writer locks work well. For recursive locks, we need to resort to |
compnerd
force-pushed
the
windows
branch
2 times, most recently
from
591c98f to
e9a6fe8
Compare
|
Once #1808 is merged, this change becomes primarily locking with a small sprinkling of TLS. We can then focus on getting that right and finishing up the Windows port for CF. |
Use `CFLock_t` instead of `pthread_mutex_t`. This has a small semantic change on Linux where we now use pthreads rather than a cmpxchg to implement locking. This makes the behaviour of POSIX-y systems and Darwin more uniform. Additionally, we now generically target pthreads on any system that supports POSIX threading. Windows continues to behave differently (though should replace the cmpxchg in favour of SWRLOCK). This leaves the open question of why do we not use `pthread_spinlock_t` to implement the libkern `OSSpinLock*` family of functions. If we do so, we should be able to fold away the differences on POSIX and Darwin systems.
__CFLock, __CFUnlock are wrappers over pthread where applicable. This now works towards switching towards Windows threading on Windows.
Replace the `PTHREAD_MUTEX_INITIALIZER` uses with the internal macro `CFLockInit`.
Implement os_unfair_lock using SRWLOCK.
Update the macro to `CF_LOCK_INIT_FOR_STRUCT`. This repairs the windows build.
Replace CFLock_t with CRITICAL_SECTION on Windows for the RunLoops. The runloops require a recursive mutex which cannot be fulfilled with SRWLOCKs, so we need to replace the locks on Windows. Ideally, we would create a CFRecusiveLock_t.
Windows is not a POSIX environment and does not provide POSIX threads.
The previous error downgrade exposes the fact that the callback had a new parameter added that we did not provide on Windows. Provide a placeholder for now.
Windows does not have the same semantics for the thread clean up when using FLS (TLS does not support callbacks on destructor). Simply wipe out everything on attempt one.
No description provided.