[Twisted-Python] Re: [Twisted-commits] r13446 - After a lengthy discussion, revert to previous threaded behavior wrt registering as the IO thread
On Apr 3, 2005, at 1:58 AM, Jp Calderone wrote:
After a lengthy discussion, revert to previous threaded behavior wrt registering as the IO thread
Modified: trunk/twisted/internet/posixbase.py ======================================================================= ======= --- trunk/twisted/internet/posixbase.py (original) +++ trunk/twisted/internet/posixbase.py Sat Apr 2 23:58:11 2005 @@ -155,7 +155,6 @@ ReactorBase.__init__(self) if self.usingThreads or platformType == "posix": self.installWaker() - threadable.whenThreaded(threadable.registerAsIOThread)
def _handleSignals(self): """Install the signal handlers for the Twisted event loop."""
You **do** realize this means isInIOThread always returns false after threading is enabled, if the reactor was started before threading was enabled, right? Something like the following should be run in the test suite. It has to be in a separate process, though, to ensure it's in the incorrect state, so something needs to be done about passing in the reactor to run, also. from twisted.python import threadable from twisted.internet import reactor def testIsInIOThread(): print "1)", threadable.isInIOThread() threadable.init(1) print "2)", threadable.isInIOThread() reactor.crash() reactor.callWhenRunning(testIsInIOThread) reactor.run()
@@ -202,10 +201,6 @@ self.startRunning(installSignalHandlers=installSignalHandlers) self.mainLoop()
- def iterate(self, delay=0): - threadable.registerAsIOThread() - ReactorBase.iterate(self, delay) - def mainLoop(self): while self.running: try:
iterate() is the external API, I think it, like startRunning, should also ensure the thread it's being called on is marked as the IO thread. James
On Sun, 3 Apr 2005 13:31:08 -0400, James Y Knight <foom@fuhm.net> wrote:
On Apr 3, 2005, at 1:58 AM, Jp Calderone wrote:
After a lengthy discussion, revert to previous threaded behavior wrt registering as the IO thread
Modified: trunk/twisted/internet/posixbase.py ======================================================================= ======= --- trunk/twisted/internet/posixbase.py (original) +++ trunk/twisted/internet/posixbase.py Sat Apr 2 23:58:11 2005 @@ -155,7 +155,6 @@ ReactorBase.__init__(self) if self.usingThreads or platformType == "posix": self.installWaker() - threadable.whenThreaded(threadable.registerAsIOThread)
def _handleSignals(self): """Install the signal handlers for the Twisted event loop."""
You **do** realize this means isInIOThread always returns false after threading is enabled, if the reactor was started before threading was enabled, right?
Yes. Fortunately this never actually happens. When it becomes possible, we can think about fixing this behavior.
Something like the following should be run in the test suite. It has to be in a separate process, though, to ensure it's in the incorrect state, so something needs to be done about passing in the reactor to run, also.
from twisted.python import threadable from twisted.internet import reactor
def testIsInIOThread(): print "1)", threadable.isInIOThread() threadable.init(1) print "2)", threadable.isInIOThread() reactor.crash()
reactor.callWhenRunning(testIsInIOThread) reactor.run()
Yes. A unit test like that _would_ be nice...
@@ -202,10 +201,6 @@ self.startRunning(installSignalHandlers=installSignalHandlers) self.mainLoop()
- def iterate(self, delay=0): - threadable.registerAsIOThread() - ReactorBase.iterate(self, delay) - def mainLoop(self): while self.running: try:
iterate() is the external API, I think it, like startRunning, should also ensure the thread it's being called on is marked as the IO thread.
Yes, however simply calling registerAsIOThread inside it is not a complete fix. The understanding to which I came is that the previous incorrectness was preferable to the new incorrectness. Jp
On Apr 3, 2005, at 2:50 PM, Jp Calderone wrote:
isInIOThread Yes. Fortunately this never actually happens. When it becomes possible, we can think about fixing this behavior.
Funny, because that test program shows that it does happen. And, for me at least, it pretty much always happens. The reactor starts up in non-threaded mode and then becomes threaded later.
iterate Yes, however simply calling registerAsIOThread inside it is not a complete fix. The understanding to which I came is that the previous incorrectness was preferable to the new incorrectness.
Elaboration necessary. James
[23:46] <exarkun> So, um, to back up and perhaps narrow scope slightly [23:46] <exarkun> The current code does the right thing, I think, except when the platform lacks threads [23:46] <exarkun> it does more than it needs to, probably, but it doesn't look like it ever does anything that's plain wrong [23:47] <exarkun> platforms that lack thread support need to just not have registerAsIOThread called, or registerAsIOThread needs to be a no-op on those platforms
It already is, and always has been. From The Source: def registerAsIOThread(): """Mark the current thread as responsable for I/O requests. """ global threaded global ioThread if threaded: import thread ioThread = thread.get_ident()
[23:48] <exarkun> Correction: registerAsIOThread in __init__ on a thread-supporting platform is also sub-optimal now, as it forces the import of the thread module when we may not actually need it.
But it doesn't. See above. It is *because* of the behavior of doing nothing when threadable.init(1) has not yet been called that whenThreaded(registerAsIOThread) is necessary.
[23:54] <glyph> Yes, but having iterate() behave in random, non-deterministic, broken ways in edge cases is preserving its current behavior :)
So, the reasoning for re-breaking iterate() is that iterate is also broken in some other way, and thus it should remain broken in this regard too? How does that even make a bit of sense? Of course, right now, the only place isInIOThread is ever used in twisted is in reactor's wakeUp function, and there, it does not matter (except for efficiency) that it will always return False. So, we could simplify isInIOThread to "return False" and call it a feature. A better solution may be to get rid of all this mess, and just make it so that if the thread module is available, it is used. Forget the "threaded mode" nonsense, and just make threaded-or-not something that has to be determined once, and finally, at startup. James
James Y Knight wrote:
So, the reasoning for re-breaking iterate() is that iterate is also broken in some other way, and thus it should remain broken in this regard too? How does that even make a bit of sense?
iterate() is broken as an API. Without even going into the problems with implementing it on a proactor, how does one determine when to start and stop services in a reactor that we one is calling .iterate() on? What happens when you call .stop()? .crash()? Do we need to invent an external status API so that the .iterate()-caller is "on their honor" to stop running at the appropriate time? Without sensible semantics for starting and stopping services, you can't start and stop the thread pool reliably, and, poof, you can't do anything useful with threads. These are the least of our problems. This is iterate() as an _external_ API. Internally, .doIteration() makes perfect sense for most reactors. The original use-case for .iterate() was to support screen-painting APIs, such as in a 3D game - perhaps reactors should instead have a 'callAsOftenAsPossible(callable)' method to support that instead. (callLater is a bit too expensive, I think, and besides it doesn't work when the FPU is in single-precision mode, as it is when running within a process like DirectX or certain proprietary openGL drivers on Windows). Another option is for such a graphically intensive application subclass one of the reactors and add a frame-drawing hook in doIteration. Does someone with a real use-case want to come forward? :)
A better solution may be to get rid of all this mess, and just make it so that if the thread module is available, it is used. Forget the "threaded mode" nonsense, and just make threaded-or-not something that has to be determined once, and finally, at startup.
The rationale for having this mess in the first place is that importing the 'thread' module slows Python down by about 30%, even if you don't actually run any threads. Originally Twisted could run without importing 'thread' at all even on a threaded platform - if we are going to make the default resolver be threaded, then it's not clear how we would restore that feature without leaving this mess intact and making sure that installing the chosen resolver is done very early in the process's lifetime. I'm open to other suggestions though. I definitely don't like what we've got, and I don't think it works properly.
On Apr 4, 2005, at 7:03 PM, Glyph Lefkowitz wrote:
The rationale for having this mess in the first place is that importing the 'thread' module slows Python down by about 30%, even if you don't actually run any threads. Originally Twisted could run without importing 'thread' at all even on a threaded platform - if we are going to make the default resolver be threaded, then it's not clear how we would restore that feature without leaving this mess intact and making sure that installing the chosen resolver is done very early in the process's lifetime.
(summarized conversation, many lines cut): [19:56] <foom> exarkun: how do you demonstrate that python slows down if you import thread? [19:56] <foom> python -c 'import thread; import test.pystone; test.pystone.main()' [19:56] <foom> doesn't show any slowdown vs not import thread'ing [19:57] * exarkun tries some stuff [20:13] <foom> it *does* slow down if you actually call thread.start_new_thread(lambda:0, ()) [20:14] <foom> on a new system, ever so slightly [20:14] <foom> on an old system, a lot [20:15] <foom> 31250 vs 17668 pystones on my RH73 system running py2.2 [20:15] <foom> 52631 vs 49504 pystones on my FC3 system py2.3 [20:16] <foom> 57471 vs 56179 pystones (on FC3 py2.4) [20:18] <exarkun> a previous point release of 2.2 or 2.3 might have moved the PyEval_InitThreads() from threadmodule's init to start_new_thread [20:21] <exarkun> hmm, no [20:21] <exarkun> PyEval_InitThreads has been in start_new_thread for 8 years :) [20:22] <exarkun> hrm [20:22] <exarkun> I suppose that doesn't tell us anything about whether another call to it was recently remove from the init function though [20:27] <foom> exarkun: Instead, I'll run pystone on python1.5.2 and have the same behavior [20:27] <foom> exarkun: and conclude that it's always been like that Conclusions: 1) We do not need to worry about importing the thread module slowing anything down. 2) We do not need to worry about using the thread module slowing anything down, on modern systems. Therefore, the rationale for the mess is invalid, and the mess can go away. James
participants (3)
-
Glyph Lefkowitz -
James Y Knight -
Jp Calderone