I recently redid how the readline module handled threads around callbacks into Python (the previous code was insane). This resulted in the following bug report: http://www.python.org/sf/1176893 Which is correctly assigned to me as it's clearly a result of my recent checkin. However, I think my code is correct and the fault lies elsewhere. Basically, if you call PyGilState_Release before PyEval_InitThreads you crash, because PyEval_ReleaseThread gets called while interpreter_lock is NULL. This is very simple to make go away -- the problem is that there are several ways! Point the first is that I really think this is a bug in the GilState APIs: the readline API isn't inherently multi-threaded and so it would be insane to call PyEval_InitThreads() in initreadline, yet it has to cope with being called in a multithreaded situation. If you can't use the GilState APIs in this situation, what are they for? Option 1) Call PyEval_ThreadsInitialized() in PyGilState_Release(). Non-invasive, but bleh. Option 2) Call PyEval_SaveThread() instead of PyEval_ReleaseThread()[1] in PyGilState_Release(). This is my favourite option (PyGilState_Ensure() calls PyEval_RestoreThread which is PyEval_SaveThread()s "mate") and I guess you can distill this long mail into the question "why doesn't PyGilState_Release do this already?" Option 3) Make PyEval_ReleaseThread() not crash when interpreter_lock == NULL. Easy, but it's actually documented that you can't do this. Opinions? Am I placing too much trust into PyGilState_Release()s existing choice of function? Cheers, mwh [1] The issue of having almost-but-not-quite identical variations of API functions -- here PyEval_AcquireThread/PyEval_ReleaseThread vs. PyEval_RestoreThread/PyEval_SaveThread -- is something I can rant about at length, if anyone is interested :) -- I located the link but haven't bothered to re-read the article, preferring to post nonsense to usenet before checking my facts. -- Ben Wolfson, comp.lang.python
Michael Hudson wrote:
Option 1) Call PyEval_ThreadsInitialized() in PyGilState_Release(). Non-invasive, but bleh.
Tim rejected this option back when PyEval_ThreadsInitialized() was added to the API [1]. Gustavo was having a similar problem with pygtk, and the end result was to add the ThreadsInitialized API so that pygtk could make its own check without slowing down the default case in the core.
Option 2) Call PyEval_SaveThread() instead of PyEval_ReleaseThread()[1] in PyGilState_Release(). This is my favourite option (PyGilState_Ensure() calls PyEval_RestoreThread which is PyEval_SaveThread()s "mate") and I guess you can distill this long mail into the question "why doesn't PyGilState_Release do this already?"
See above. Although I'm now wondering about the opposite question: Why doesn't PyGilState_Ensure use PyEval_AcquireThread? Cheers, Nick. [1] http://sourceforge.net/tracker/?func=detail&aid=1044089&group_id=5470&atid=305470 [2] http://mail.python.org/pipermail/python-dev/2004-August/047870.html -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia --------------------------------------------------------------- http://boredomandlaziness.skystorm.net
Nick Coghlan <ncoghlan@gmail.com> writes:
Michael Hudson wrote:
Option 1) Call PyEval_ThreadsInitialized() in PyGilState_Release(). Non-invasive, but bleh.
Tim rejected this option back when PyEval_ThreadsInitialized() was added to the API [1].
Well, not really. The patch that was rejected was much larger than any proposal of mine. My option 1) is this: --- pystate.c 09 Feb 2005 10:56:18 +0000 2.39 +++ pystate.c 07 Apr 2005 13:19:55 +0100 @@ -502,7 +502,8 @@ PyThread_delete_key_value(autoTLSkey); } /* Release the lock if necessary */ - else if (oldstate == PyGILState_UNLOCKED) - PyEval_ReleaseThread(tcur); + else if (oldstate == PyGILState_UNLOCKED + && PyEval_ThreadsInitialized()) + PyEval_ReleaseThread(); } #endif /* WITH_THREAD */
Gustavo was having a similar problem with pygtk, and the end result was to add the ThreadsInitialized API so that pygtk could make its own check without slowing down the default case in the core.
Well, Gustavo seemed to be complaining about the cost of the locking. I'm complaining about crashes.
Option 2) Call PyEval_SaveThread() instead of PyEval_ReleaseThread()[1] in PyGilState_Release(). This is my favourite option (PyGilState_Ensure() calls PyEval_RestoreThread which is PyEval_SaveThread()s "mate") and I guess you can distill this long mail into the question "why doesn't PyGilState_Release do this already?"
This option corresponds to this patch: --- pystate.c 09 Feb 2005 10:56:18 +0000 2.39 +++ pystate.c 07 Apr 2005 13:24:33 +0100 @@ -503,6 +503,6 @@ } /* Release the lock if necessary */ else if (oldstate == PyGILState_UNLOCKED) - PyEval_ReleaseThread(tcur); + PyEval_SaveThread(); } #endif /* WITH_THREAD */
See above. Although I'm now wondering about the opposite question: Why doesn't PyGilState_Ensure use PyEval_AcquireThread?
Well, that would make more sense than what we have now. OTOH, I'd *much* rather make the PyGilState functions more tolerant -- I thought being vaguely easy to use was part of their point. I fail to believe the patch associated with option 2) has any detectable performance cost. Cheers, mwh -- People think I'm a nice guy, and the fact is that I'm a scheming, conniving bastard who doesn't care for any hurt feelings or lost hours of work if it just results in what I consider to be a better system. -- Linus Torvalds
[Michael Hudson]
... Point the first is that I really think this is a bug in the GilState APIs: the readline API isn't inherently multi-threaded and so it would be insane to call PyEval_InitThreads() in initreadline, yet it has to cope with being called in a multithreaded situation. If you can't use the GilState APIs in this situation, what are they for?
That's explained in the PEP -- of course <wink>: http://www.python.org/peps/pep-0311.html Under "Limitations and Exclusions" it specifically disowns responsibility for worrying about whether Py_Initialize() and PyEval_InitThreads() have been called: This API will not perform automatic initialization of Python, or initialize Python for multi-threaded operation. Extension authors must continue to call Py_Initialize(), and for multi-threaded applications, PyEval_InitThreads(). The reason for this is that the first thread to call PyEval_InitThreads() is nominated as the "main thread" by Python, and so forcing the extension author to specify the main thread (by forcing her to make this first call) removes ambiguity. As Py_Initialize() must be called before PyEval_InitThreads(), and as both of these functions currently support being called multiple times, the burden this places on extension authors is considered reasonable. That doesn't mean there isn't a clever way to get the same effect anyway, but I don't have time to think about it, and reassigned the bug report to Mark (who may or may not have time).
Tim Peters <tim.peters@gmail.com> writes:
[Michael Hudson]
... Point the first is that I really think this is a bug in the GilState APIs: the readline API isn't inherently multi-threaded and so it would be insane to call PyEval_InitThreads() in initreadline, yet it has to cope with being called in a multithreaded situation. If you can't use the GilState APIs in this situation, what are they for?
That's explained in the PEP -- of course <wink>:
Gnarr. Of course, I read this passage. I think it's missing a use case.
Under "Limitations and Exclusions" it specifically disowns responsibility for worrying about whether Py_Initialize() and PyEval_InitThreads() have been called:
[snip quote] This suggests that I should call PyEval_InitThreads() in initreadline(), which seems daft.
That doesn't mean there isn't a clever way to get the same effect anyway,
Pah. There's a very simple way (see my reply to Nick). It even works in the case that PyEval_InitThreads() is called in between the call to PyGilState_Ensure() and PyGilState_Release().
but I don't have time to think about it, and reassigned the bug report to Mark (who may or may not have time).
He gets a week :) Cheers, mwh -- Or here's an even simpler indicator of how much C++ sucks: Print out the C++ Public Review Document. Have someone hold it about three feet above your head and then drop it. Thus you will be enlightened. -- Thant Tessman
Under "Limitations and Exclusions" it specifically disowns responsibility for worrying about whether Py_Initialize() and PyEval_InitThreads() have been called:
[snip quote]
This suggests that I should call PyEval_InitThreads() in initreadline(), which seems daft.
fwiw, Modules/_bsddb.c does exactly that. -g
"Gregory P. Smith" <greg@electricrain.com> writes:
Under "Limitations and Exclusions" it specifically disowns responsibility for worrying about whether Py_Initialize() and PyEval_InitThreads() have been called:
[snip quote]
This suggests that I should call PyEval_InitThreads() in initreadline(), which seems daft.
fwiw, Modules/_bsddb.c does exactly that.
Interesting. The problem with readline.c doing this is that it gets implicitly imported by the interpreter -- although only for interactive sessions. Maybe that's not that big a deal. I'd still prefer to change the functions (would updating the PEP be in order here? Obviously, I'd update the api documentation). Cheers, mwh -- It's relatively seldom that desire for sex is involved in technology procurement decisions. -- ESR at EuroPython 2002
On Apr 9, 2005, at 11:15 AM, Michael Hudson wrote:
"Gregory P. Smith" <greg@electricrain.com> writes:
Under "Limitations and Exclusions" it specifically disowns responsibility for worrying about whether Py_Initialize() and PyEval_InitThreads() have been called:
[snip quote]
This suggests that I should call PyEval_InitThreads() in initreadline(), which seems daft.
fwiw, Modules/_bsddb.c does exactly that.
Interesting. The problem with readline.c doing this is that it gets implicitly imported by the interpreter -- although only for interactive sessions. Maybe that's not that big a deal. I'd still prefer to change the functions (would updating the PEP be in order here? Obviously, I'd update the api documentation).
Is there a good reason to *not* call PyEval_InitThreads when using a threaded Python? Sounds like it would just be easier to implicitly call it during Py_Initialize some day. -bob
Bob Ippolito <bob@redivi.com> writes:
Is there a good reason to *not* call PyEval_InitThreads when using a threaded Python?
Well, it depends how expensive ones OS's locking primitives are, I think. There were some numbers posted to the twisted list recently that showed it didn't make a whole lot of difference on some platform or other... I don't have the knowledge or the courage to make that call.
Sounds like it would just be easier to implicitly call it during Py_Initialize some day.
That might indeed be simpler. Cheers, mwh -- The gripping hand is really that there are morons everywhere, it's just that the Americon morons are funnier than average. -- Pim van Riezen, alt.sysadmin.recovery
On Apr 10, 2005, at 11:22 AM, Michael Hudson wrote:
Bob Ippolito <bob@redivi.com> writes:
Is there a good reason to *not* call PyEval_InitThreads when using a threaded Python?
Well, it depends how expensive ones OS's locking primitives are, I think. There were some numbers posted to the twisted list recently that showed it didn't make a whole lot of difference on some platform or other... I don't have the knowledge or the courage to make that call.
Sounds like it would just be easier to implicitly call it during Py_Initialize some day.
That might indeed be simpler.
Here's the numbers. It looks like something changed between python 2.2 and 2.3 that made calling PyEval_InitThreads a lot less expensive. So, it doesn't seem to make a whole lot of difference on recent versions of Python. Three test programs: ${PYTHON} -c 'import pystone, time; print pystone.pystones(200000)' ${PYTHON} -c 'import thread, pystone, time; print pystone.pystones(200000)' ${PYTHON} -c 'import thread, pystone, time; thread.start_new_thread(lambda: time.sleep(10000), ()); print pystone.pystones(200000)' All tests run using the same copy of pystone. System 1: RH73, dual 3GHz Xeon [GCC 2.96 20000731 (Red Hat Linux 7.3 2.96-110)] -------- Python 1.5.2 (#1, Apr 3 2002, 18:16:26) (8.15, 24540) (8.28, 24155) (12.78, 15649) Python 2.2.2 (#1, Jul 23 2003, 13:47:48) (6.32, 31646) (6.27, 31898) (11.1, 18018) Python 2.4.1 (#1, Apr 4 2005, 17:19:27) (4.60, 43478) (4.61, 43384) (4.74, 42194) System 2, FC3/64, dual 2.4GHz athlon 64. [GCC 3.4.2 20041017 (Red Hat 3.4.2-6.fc3)] -------- Python 2.3.4 (#1, Oct 26 2004, 16:45:38) (3.84, 52083) (3.80, 52632) (3.98, 50251) Python 2.4.1 (#1, Apr 10 2005, 15:47:53) (3.09, 64725) (3.08, 64935) (3.26, 61350) Python 2.4.1 (#1, Apr 1 2005, 16:45:07) *compiled in 32 bit mode* (3.35, 59701) (3.42, 58480) (3.57, 56022)
James Y Knight <foom@fuhm.net> writes:
On Apr 10, 2005, at 11:22 AM, Michael Hudson wrote:
Bob Ippolito <bob@redivi.com> writes:
Is there a good reason to *not* call PyEval_InitThreads when using a threaded Python?
Well, it depends how expensive ones OS's locking primitives are, I think. There were some numbers posted to the twisted list recently that showed it didn't make a whole lot of difference on some platform or other... I don't have the knowledge or the courage to make that call.
Sounds like it would just be easier to implicitly call it during Py_Initialize some day.
That might indeed be simpler.
Here's the numbers. It looks like something changed between python 2.2 and 2.3 that made calling PyEval_InitThreads a lot less expensive. So, it doesn't seem to make a whole lot of difference on recent versions of Python.
Thanks. I see similar results for 2.3 and 2.4 on OS X (don't have 2.2 here). It's very much a guess, but could this patch: [ 525532 ] Add support for POSIX semaphores be the one to thank? Cheers, mwh -- Now this is what I don't get. Nobody said absolutely anything bad about anything. Yet it is always possible to just pull random flames out of ones ass. -- http://www.advogato.org/person/vicious/diary.html?start=60
On Apr 10, 2005, at 2:48 PM, Michael Hudson wrote:
James Y Knight <foom@fuhm.net> writes:
On Apr 10, 2005, at 11:22 AM, Michael Hudson wrote:
Bob Ippolito <bob@redivi.com> writes:
Is there a good reason to *not* call PyEval_InitThreads when using a threaded Python?
Well, it depends how expensive ones OS's locking primitives are, I think. There were some numbers posted to the twisted list recently that showed it didn't make a whole lot of difference on some platform or other... I don't have the knowledge or the courage to make that call.
Sounds like it would just be easier to implicitly call it during Py_Initialize some day.
That might indeed be simpler.
Here's the numbers. It looks like something changed between python 2.2 and 2.3 that made calling PyEval_InitThreads a lot less expensive. So, it doesn't seem to make a whole lot of difference on recent versions of Python.
Thanks. I see similar results for 2.3 and 2.4 on OS X (don't have 2.2 here).
It's very much a guess, but could this patch:
[ 525532 ] Add support for POSIX semaphores
be the one to thank?
No, Mac OS X doesn't implement POSIX semaphores. -bob
Bob Ippolito <bob@redivi.com> writes:
On Apr 10, 2005, at 2:48 PM, Michael Hudson wrote:
James Y Knight <foom@fuhm.net> writes:
Here's the numbers. It looks like something changed between python 2.2 and 2.3 that made calling PyEval_InitThreads a lot less expensive. So, it doesn't seem to make a whole lot of difference on recent versions of Python.
Thanks. I see similar results for 2.3 and 2.4 on OS X (don't have 2.2 here).
It's very much a guess, but could this patch:
[ 525532 ] Add support for POSIX semaphores
be the one to thank?
No, Mac OS X doesn't implement POSIX semaphores.
Well, does OS X show the same effect between 2.2 and 2.3? I don't have a 2.2 on OS X any more, I was just talking about James' results on linux. Cheers, mwh -- Slim Shady is fed up with your shit, and he's going to kill you. -- Eminem, "Public Service Announcement 2000"
On Apr 10, 2005, at 4:08 PM, Michael Hudson wrote:
Bob Ippolito <bob@redivi.com> writes:
On Apr 10, 2005, at 2:48 PM, Michael Hudson wrote:
James Y Knight <foom@fuhm.net> writes:
Here's the numbers. It looks like something changed between python 2.2 and 2.3 that made calling PyEval_InitThreads a lot less expensive. So, it doesn't seem to make a whole lot of difference on recent versions of Python.
Thanks. I see similar results for 2.3 and 2.4 on OS X (don't have 2.2 here).
It's very much a guess, but could this patch:
[ 525532 ] Add support for POSIX semaphores
be the one to thank?
No, Mac OS X doesn't implement POSIX semaphores.
Well, does OS X show the same effect between 2.2 and 2.3? I don't have a 2.2 on OS X any more, I was just talking about James' results on linux.
I don't have 2.2 on OS X any more, either. -bob
participants (6)
-
Bob Ippolito -
Gregory P. Smith -
James Y Knight -
Michael Hudson -
Nick Coghlan -
Tim Peters