Roundup Robot added the comment:
New changeset 08b87dda6f6a by Kristján Valur Jónsson in branch 'default':
Issue #15038 : Fixing the condition broadcast and docs.
http://hg.python.org/cpython/rev/08b87dda6f6a
New changeset fde60d3f542e by Kristján Valur Jónsson in branch '3.3':
Issue #15038 :
Changes by Kristján Valur Jónsson krist...@ccpgames.com:
--
resolution: - fixed
status: open - closed
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
___
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Thanks, Benjamin. That's what reviews are for :)
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
___
Benjamin Peterson benja...@python.org added the comment:
I see dead code here:
Py_LOCAL_INLINE(int)
PyCOND_BROADCAST(PyCOND_T *cv)
{
if (cv-waiting 0) {
return ReleaseSemaphore(cv-sem, cv-waiting, NULL) ? 0 : -1;
cv-waiting = 0;
}
return 0;
}
--
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Thanks.
The comments remain as the last bit of the original Condition Variable
emulation code that was put in place for the new GIL.
--
___
Python tracker rep...@bugs.python.org
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Thanks Antoine. I tested this in my virtualbox so something new must have
happened... Anyway, the GIL code should not have changed from before, only
moved about slightly. I´ll figure out what happened
--
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
You are right, Richard. Thanks for pointing this out. This is not a new
problem, however, because this code has been in the New GIL since it was
launched.
The purpose of the n_waiting member is to make signal a no-op when no
Richard Oudkerk shibt...@gmail.com added the comment:
The old version was
243 __inline static void _cond_signal(COND_T *cond) {
244 /* NOTE: This must be called with the mutex held */
245 if (cond-n_waiting 0) {
246 if (!ReleaseSemaphore(cond-sem, 1, NULL))
247
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Well spotted. This probably fixes the failure we saw in the buildbots as well.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
Roundup Robot devn...@psf.upfronthosting.co.za added the comment:
New changeset 110b38c36a31 by Kristjan Valur Jonsson in branch 'default':
Issue #15038:
http://hg.python.org/cpython/rev/110b38c36a31
--
___
Python tracker rep...@bugs.python.org
Richard Oudkerk shibt...@gmail.com added the comment:
Standard condition variables have the following guarantees:
* if there are any waiters then signal()/notify() will awaken at least one of
them;
* if there are any waiters then broadcast()/notify_all() will awaken all of
them.
The
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Yes, another correct observation. This can if a thread is interrupted in
between releasing the mutex and waiting for the semaphore.
I see no way around this apart from manually creating event objects for every
thread.
Personally
Antoine Pitrou pit...@free.fr added the comment:
Personally I'm not sure it is a wise guarantee to make.
If you make sure internal users are immune to this issue, then fine (but make
sure to document it somewhere).
However, if this lost wakeup problem can affect current users of the API, then
Antoine Pitrou pit...@free.fr added the comment:
However, if this lost wakeup problem can affect current users of the API,
then it sounds unacceptable.
Let me elaborate: the GIL can perhaps suffer lost wakeups from time to time.
The Lock API certainly shouldn't.
--
Richard Oudkerk shibt...@gmail.com added the comment:
Let me elaborate: the GIL can perhaps suffer lost wakeups from time to
time. The Lock API certainly shouldn't.
I think with FORCE_SWITCHING defined (the default?) it is not possible for the
thread releasing the GIL to immediately
Richard Oudkerk shibt...@gmail.com added the comment:
The implementation in condvar.h is basically the same as one of the attempts
mentioned in
http://birrell.org/andrew/papers/ImplementingCVs.pdf
(Listing 2 fixed to use non-binary semaphores.)
The implementation for
Antoine Pitrou pit...@free.fr added the comment:
The implementation for multiprocessing.Condition is virtually the same
as Listing 3 which the author says he thinks is formally correct but
with a fundamental performance problem.
To me, it seems similar to the last listing (under The
Antoine Pitrou pit...@free.fr added the comment:
The problem Richard describes isn´t a lost wakeup. PyCOND_SIGNAL
_will_ wake up _at least_ one thread. It just isn't guaranteed to be
one of those who previously called PyCOND_WAIT(): It could be a
latecomer to the game, including the one
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
The problem Richard describes isn´t a lost wakeup. PyCOND_SIGNAL _will_ wake
up _at least_ one thread. It just isn't guaranteed to be one of those who
previously called PyCOND_WAIT(): It could be a latecomer to the game,
Antoine Pitrou pit...@free.fr added the comment:
The implementation for multiprocessing.Condition is virtually the same
as Listing 3 which the author says he thinks is formally correct but
with a fundamental performance problem.
To me, it seems similar to the last listing (under The
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
It's an interesting article Richard, but I don't see how their 2nd attempt
solves the probvlem. All it does is block the thread doing the Signal(), not
other threads, from stealing the wakeup.
I think I know how to fix this
Richard Oudkerk shibt...@gmail.com added the comment:
The notes should also mention that PyCOND_SIGNAL() and PyCOND_BROADCAST() must
be called while holding the mutex. (pthreads does not have that restriction.)
--
___
Python tracker
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Right.
Without holding the mutex, the definition of already blocked is of course
meaningless, since only the holding the mutex can define any ordering.
pthread standard indeed says however, if predictable scheduling behaviour is
Roundup Robot devn...@psf.upfronthosting.co.za added the comment:
New changeset d7a72fdcc168 by Kristjan Valur Jonsson in branch 'default':
Issue #15038: Document caveats with the emulated condition variables.
http://hg.python.org/cpython/rev/d7a72fdcc168
--
Richard Oudkerk shibt...@gmail.com added the comment:
It's an interesting article Richard, but I don't see how their 2nd attempt
solves the problem. All it does is block the thread doing the Signal(),
not other threads, from stealing the wakeup.
Do you mean the listing on page 5? (The
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Ah, right, the lock x, I forgot about that.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
___
Richard Oudkerk shibt...@gmail.com added the comment:
1.41 Generic emulations of the pthread_cond_* API using
1.42 earlier Win32 functions can be found on the Web.
1.43 The following read can be edificating (or not):
1.44
Roundup Robot devn...@psf.upfronthosting.co.za added the comment:
New changeset 978326f98316 by Kristján Valur Jónsson in branch 'default':
Issue #15038: Optimize python Locks on Windows
http://hg.python.org/cpython/rev/978326f98316
--
nosy: +python-dev
Antoine Pitrou pit...@free.fr added the comment:
There's a problem here:
Fatal Python error: PyCOND_SIGNAL(gil_cond) failed
http://www.python.org/dev/buildbot/all/builders/x86%20XP-4%203.x/builds/6859/steps/test/logs/stdio
--
___
Python tracker
Richard Oudkerk shibt...@gmail.com added the comment:
Py_LOCAL_INLINE(int)
_PyCOND_WAIT_MS(PyCOND_T *cv, PyMUTEX_T *cs, DWORD ms)
{
DWORD wait;
cv-waiting++;
PyMUTEX_UNLOCK(cs);
/* lost wakeup bug would occur if the caller were interrupted here,
* but we are safe because we
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Ok, I take the lack of negative reviews as a general approvement. I'll improve
comments a bit, write the appropriate NEWS item and make a commit soon.
--
___
Python tracker
Changes by STINNER Victor victor.stin...@gmail.com:
--
nosy: +haypo
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
___
___
Python-bugs-list
Changes by Antoine Pitrou pit...@free.fr:
--
superseder: Locks broken wrt timeouts on Windows -
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
___
Changes by Antoine Pitrou pit...@free.fr:
--
stage: - patch review
type: enhancement - performance
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
___
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
While I'm confident about the correctness of this implementation (it´s in
production use right now) I´d like comments on the architecture.
- Are people comfortable with the notion of an include file with an inline
implementation
Paul Moore p.f.mo...@gmail.com added the comment:
Applies and builds cleanly on Win7 32-bit. The speed difference is visible here
too:
PS D:\Data\cpython\PCbuild .\python.exe -m timeit -s from _thread import
allocate_lock; l=allocate_lock() l.acquire();l.release()
100 loops, best of 3:
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
I've tested Ubuntu 64 myself using a Virtualbox, confirming that the pythread
functionality is untouched.
(funny how those vi keystrokes seem to be embedded into your amygdala after
decades of disuse)
--
New submission from Kristján Valur Jónsson krist...@ccpgames.com:
The attached patch does three things:
- Abstract the condition variable used by ceval_gil.h into a separate file,
condvar.h. It now defines a PyMUTEX_T, PyCOND_T and associated functions.
This file can be used by different
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
This defect springs out of issue #11618
--
superseder: - Locks broken wrt timeouts on Windows
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15038
39 matches
Mail list logo
