[Twisted-Python] A problem with runUntilCurrent
Glyph, I couldn't resist digging into the code you sent. I noticed that you changed the code to use callLater. But there's a problem with that, I suspect... In base.runUntilCurrent, there's this while loop... while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): call = self._pendingTimedCalls.pop() . . . This means that when you do a callLater with a delay of 0, runUntilCurrent will immediately call the delayed function, which then calls callLater, and again the delayed function is immediately called, etc. So the net result of the timing test you ran was that the reactor main loop just hung until the test was completed. So it ran fast. But it really didn't run at all. ;-( Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
Bill la Forge wrote:
No, callLater(0,) doesn't call the function synchronously, it calls it in the next reactor iteration. i.e., not "*right* now", but "ASAP". -- Twisted | Christopher Armstrong: International Man of Twistery Radix | Release Manager, Twisted Project ---------+ http://radix.twistedmatrix.com/
On Mon, 2004-05-17 at 10:20, Christopher Armstrong wrote:
No, callLater(0,) doesn't call the function synchronously, it calls it in the next reactor iteration. i.e., not "*right* now", but "ASAP".
That's what it's *supposed* to do. But Glyph's code had a bug for the following case: def infinite(): reactor.callLater(0, infinite) reactor.callLater(0, infinite) where it would just call infinite() over and over and over and over. -- Itamar Shtull-Trauring http://itamarst.org
Itamar, This is also problematic. What this code does is limit the number of calls that will be made by runUntilCurrent. However, if there's code that does a callLater with a delay of 0, it will always execute that. The problem now is that only callLater's with a delay of 0 will be executed. Yes, now the main loop runs fine, but the other timeouts are prevented from running. I'm thinking that what you need to do is to have runUntilCurrent first extract all the pending timed calls that it plans to execute, creating a sub-list, and then call them. It takes two loops, not one. Bill Itamar Shtull-Trauring <itamar@itamarst.org> wrote: On Mon, 2004-05-17 at 10:14, Bill la Forge wrote:
And that's why testImmediateThread failed. Attached is a fixed up patch that solves that problem. Index: base.py =================================================================== --- base.py (revision 10658) +++ base.py (working copy) @@ -368,7 +368,11 @@ assert callable(_f), "%s is not callable" % _f assert sys.maxint >= _seconds >= 0, \ "%s is not greater than or equal to 0 seconds" % (_seconds,) - tple = DelayedCall(seconds() + _seconds, _f, args, kw, + if _seconds == 0: + tcc = 0 + else: + tcc = seconds() + _seconds + tple = DelayedCall(tcc, _f, args, kw, self._pendingTimedCalls.remove, self._resetCallLater) insort(self._pendingTimedCalls, tple) @@ -416,7 +420,10 @@ count += 1 del self.threadCallQueue[:count] now = seconds() - while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): + # make sure we don't run newly added calls, so we don't get into infinite loop + numCalls = len(self._pendingTimedCalls) + while numCalls and (self._pendingTimedCalls[-1].time <= now): + numCalls -= 1 call = self._pendingTimedCalls.pop() try: call.called = 1 Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
On Mon, 2004-05-17 at 11:04, Bill la Forge wrote:
Ah yes. I'll look into doing that later. -- Itamar Shtull-Trauring http://itamarst.org
At 04:04 PM 5/17/04 +0100, Bill la Forge wrote:
What I don't understand is why that should be the case. A delay of zero should equal the *current time* plus zero. The 'now' variable, however, is fixed at the time when 'runUntilCurrent()' is called. Therefore, as soon as any time has elapsed (i.e. time() advances one quantum), a newly scheduled callLater(0,) must be scheduled for a time that is *after* 'now', and therefore ineligible for call within the loop. (I'm particularly puzzled, therefore, by the part of Itamar's patch that sets the scheduled time to zero for callLater(0,).) The real problem here, if any, is likely the use of a low-resolution time() value. On Windows, for example, the tick granularity is like 1/18th of a second, so a callLater(0,) loop will consume a huge amount of time. On Windows, a scheduler should probably use the high-res clock() rather than the low-res time() in order to work around this, which will ensure that nothing scheduled during a 'runUntilCurrent()' call will be executed until the *next* 'runUntilCurrent()' call.
A better than perfect patch! Itamar, your latest patch has its flaws, as things are a bit off when a non-zero delay is passed via callLater when callLater is called from a delayed call. But any mistakes it makes are quickly fixed in the next cycle, and this is likely an uncommon case as well. Of greater importance than being perfect, then, is that your patch is likely faster than a perfict patch would be. And this is one place where speed is most critical. I'll be doing another release in a day or so, and include this patch. Thanks to you and the rest of the Twisted development team who took the time to dig into my code and then make the changes to Twisted! Itamar Shtull-Trauring <itamar@itamarst.org> wrote: Another attempt at a patch. Index: base.py Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
On Mon, 2004-05-17 at 22:34, Bill la Forge wrote:
Actually it's buggy in the phase of cancel(), and is failing the twisted test suite... -- Itamar Shtull-Trauring http://itamarst.org
OK, then I'll hold off on my release. Thanks for the heads up! Bill Itamar Shtull-Trauring <itamar@itamarst.org> wrote: On Mon, 2004-05-17 at 22:34, Bill la Forge wrote:
Actually it's buggy in the phase of cancel(), and is failing the twisted test suite... -- Itamar Shtull-Trauring http://itamarst.org _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
Bill la Forge wrote:
You're welcome :). The reason that we did so is because your run-time patch requires talking to the reactor over private interfaces. Normally this sort of thing can be overlooked in Python, but there are actually multiple implementations of the reactor, some of which may not work with those changes. So a more efficient implementation of callLater(0,...) will allow your code to be compatible with Twisted and not just happen to work on the default reactor :).
I'm not much good at patch files--lacking the tools. But take a look at the attached hack (crafted manually). It works for me, with luck it should pass your regression tests. (Its got the required 2 loops I was talking about.) I'll note in passing that I'm only getting a measure of 7K with this code. But that's probably as good as we can get without my patches. Good enough, I suspect. Bill Glyph Lefkowitz <glyph@divmod.com> wrote: Bill la Forge wrote:
You're welcome :). The reason that we did so is because your run-time patch requires talking to the reactor over private interfaces. Normally this sort of thing can be overlooked in Python, but there are actually multiple implementations of the reactor, some of which may not work with those changes. So a more efficient implementation of callLater(0,...) will allow your code to be compatible with Twisted and not just happen to work on the default reactor :). _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python Yahoo! India Matrimony: Find your partner online. Index: base.py =================================================================== --- base.py (revision 10658) +++ base.py (working copy) @@ -368,7 +368,11 @@ assert callable(_f), "%s is not callable" % _f assert sys.maxint >= _seconds >= 0, \ "%s is not greater than or equal to 0 seconds" % (_seconds,) - tple = DelayedCall(seconds() + _seconds, _f, args, kw, + if _seconds == 0: + tcc = 0 + else: + tcc = seconds() + _seconds + tple = DelayedCall(tcc, _f, args, kw, self._pendingTimedCalls.remove, self._resetCallLater) insort(self._pendingTimedCalls, tple) @@ -416,8 +420,11 @@ count += 1 del self.threadCallQueue[:count] now = seconds() - while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): - call = self._pendingTimedCalls.pop() + while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): + do.append(self._pendingTimedCalls.pop()) + for call in do: try: call.called = 1 call.func(*call.args, **call.kw)
A few corrections: 1. I left the do=[] statment out of the patch. Its been added. (I may still have made a format error in this hand-crafted patch, so please take care!) 2. Most of the speed loss now is due to changes in my code for supporting thread migration. So speed is not an issue. Bill Bill la Forge <laforge49@yahoo.co.in> wrote: I'm not much good at patch files--lacking the tools. But take a look at the attached hack (crafted manually). It works for me, with luck it should pass your regression tests. (Its got the required 2 loops I was talking about.) I'll note in passing that I'm only getting a measure of 7K with this code. But that's probably as good as we can get without my patches. Good enough, I suspect. Bill Yahoo! India Matrimony: Find your partner online. Index: base.py =================================================================== --- base.py (revision 10658) +++ base.py (working copy) @@ -368,7 +368,11 @@ assert callable(_f), "%s is not callable" % _f assert sys.maxint >= _seconds >= 0, \ "%s is not greater than or equal to 0 seconds" % (_seconds,) - tple = DelayedCall(seconds() + _seconds, _f, args, kw, + if _seconds == 0: + tcc = 0 + else: + tcc = seconds() + _seconds + tple = DelayedCall(tcc, _f, args, kw, self._pendingTimedCalls.remove, self._resetCallLater) insort(self._pendingTimedCalls, tple) @@ -416,8 +420,11 @@ count += 1 del self.threadCallQueue[:count] now = seconds() - while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): - call = self._pendingTimedCalls.pop() + do=[] + while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): + do.append(self._pendingTimedCalls.pop()) + for call in do: try: call.called = 1 call.func(*call.args, **call.kw)
Bill la Forge wrote:
No, callLater(0,) doesn't call the function synchronously, it calls it in the next reactor iteration. i.e., not "*right* now", but "ASAP". -- Twisted | Christopher Armstrong: International Man of Twistery Radix | Release Manager, Twisted Project ---------+ http://radix.twistedmatrix.com/
On Mon, 2004-05-17 at 10:20, Christopher Armstrong wrote:
No, callLater(0,) doesn't call the function synchronously, it calls it in the next reactor iteration. i.e., not "*right* now", but "ASAP".
That's what it's *supposed* to do. But Glyph's code had a bug for the following case: def infinite(): reactor.callLater(0, infinite) reactor.callLater(0, infinite) where it would just call infinite() over and over and over and over. -- Itamar Shtull-Trauring http://itamarst.org
Itamar, This is also problematic. What this code does is limit the number of calls that will be made by runUntilCurrent. However, if there's code that does a callLater with a delay of 0, it will always execute that. The problem now is that only callLater's with a delay of 0 will be executed. Yes, now the main loop runs fine, but the other timeouts are prevented from running. I'm thinking that what you need to do is to have runUntilCurrent first extract all the pending timed calls that it plans to execute, creating a sub-list, and then call them. It takes two loops, not one. Bill Itamar Shtull-Trauring <itamar@itamarst.org> wrote: On Mon, 2004-05-17 at 10:14, Bill la Forge wrote:
And that's why testImmediateThread failed. Attached is a fixed up patch that solves that problem. Index: base.py =================================================================== --- base.py (revision 10658) +++ base.py (working copy) @@ -368,7 +368,11 @@ assert callable(_f), "%s is not callable" % _f assert sys.maxint >= _seconds >= 0, \ "%s is not greater than or equal to 0 seconds" % (_seconds,) - tple = DelayedCall(seconds() + _seconds, _f, args, kw, + if _seconds == 0: + tcc = 0 + else: + tcc = seconds() + _seconds + tple = DelayedCall(tcc, _f, args, kw, self._pendingTimedCalls.remove, self._resetCallLater) insort(self._pendingTimedCalls, tple) @@ -416,7 +420,10 @@ count += 1 del self.threadCallQueue[:count] now = seconds() - while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): + # make sure we don't run newly added calls, so we don't get into infinite loop + numCalls = len(self._pendingTimedCalls) + while numCalls and (self._pendingTimedCalls[-1].time <= now): + numCalls -= 1 call = self._pendingTimedCalls.pop() try: call.called = 1 Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
On Mon, 2004-05-17 at 11:04, Bill la Forge wrote:
Ah yes. I'll look into doing that later. -- Itamar Shtull-Trauring http://itamarst.org
At 04:04 PM 5/17/04 +0100, Bill la Forge wrote:
What I don't understand is why that should be the case. A delay of zero should equal the *current time* plus zero. The 'now' variable, however, is fixed at the time when 'runUntilCurrent()' is called. Therefore, as soon as any time has elapsed (i.e. time() advances one quantum), a newly scheduled callLater(0,) must be scheduled for a time that is *after* 'now', and therefore ineligible for call within the loop. (I'm particularly puzzled, therefore, by the part of Itamar's patch that sets the scheduled time to zero for callLater(0,).) The real problem here, if any, is likely the use of a low-resolution time() value. On Windows, for example, the tick granularity is like 1/18th of a second, so a callLater(0,) loop will consume a huge amount of time. On Windows, a scheduler should probably use the high-res clock() rather than the low-res time() in order to work around this, which will ensure that nothing scheduled during a 'runUntilCurrent()' call will be executed until the *next* 'runUntilCurrent()' call.
A better than perfect patch! Itamar, your latest patch has its flaws, as things are a bit off when a non-zero delay is passed via callLater when callLater is called from a delayed call. But any mistakes it makes are quickly fixed in the next cycle, and this is likely an uncommon case as well. Of greater importance than being perfect, then, is that your patch is likely faster than a perfict patch would be. And this is one place where speed is most critical. I'll be doing another release in a day or so, and include this patch. Thanks to you and the rest of the Twisted development team who took the time to dig into my code and then make the changes to Twisted! Itamar Shtull-Trauring <itamar@itamarst.org> wrote: Another attempt at a patch. Index: base.py Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
On Mon, 2004-05-17 at 22:34, Bill la Forge wrote:
Actually it's buggy in the phase of cancel(), and is failing the twisted test suite... -- Itamar Shtull-Trauring http://itamarst.org
OK, then I'll hold off on my release. Thanks for the heads up! Bill Itamar Shtull-Trauring <itamar@itamarst.org> wrote: On Mon, 2004-05-17 at 22:34, Bill la Forge wrote:
Actually it's buggy in the phase of cancel(), and is failing the twisted test suite... -- Itamar Shtull-Trauring http://itamarst.org _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python Bill la Forge http://www.geocities.com/laforge49/ Yahoo! India Matrimony: Find your partner online.
Bill la Forge wrote:
You're welcome :). The reason that we did so is because your run-time patch requires talking to the reactor over private interfaces. Normally this sort of thing can be overlooked in Python, but there are actually multiple implementations of the reactor, some of which may not work with those changes. So a more efficient implementation of callLater(0,...) will allow your code to be compatible with Twisted and not just happen to work on the default reactor :).
I'm not much good at patch files--lacking the tools. But take a look at the attached hack (crafted manually). It works for me, with luck it should pass your regression tests. (Its got the required 2 loops I was talking about.) I'll note in passing that I'm only getting a measure of 7K with this code. But that's probably as good as we can get without my patches. Good enough, I suspect. Bill Glyph Lefkowitz <glyph@divmod.com> wrote: Bill la Forge wrote:
You're welcome :). The reason that we did so is because your run-time patch requires talking to the reactor over private interfaces. Normally this sort of thing can be overlooked in Python, but there are actually multiple implementations of the reactor, some of which may not work with those changes. So a more efficient implementation of callLater(0,...) will allow your code to be compatible with Twisted and not just happen to work on the default reactor :). _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python Yahoo! India Matrimony: Find your partner online. Index: base.py =================================================================== --- base.py (revision 10658) +++ base.py (working copy) @@ -368,7 +368,11 @@ assert callable(_f), "%s is not callable" % _f assert sys.maxint >= _seconds >= 0, \ "%s is not greater than or equal to 0 seconds" % (_seconds,) - tple = DelayedCall(seconds() + _seconds, _f, args, kw, + if _seconds == 0: + tcc = 0 + else: + tcc = seconds() + _seconds + tple = DelayedCall(tcc, _f, args, kw, self._pendingTimedCalls.remove, self._resetCallLater) insort(self._pendingTimedCalls, tple) @@ -416,8 +420,11 @@ count += 1 del self.threadCallQueue[:count] now = seconds() - while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): - call = self._pendingTimedCalls.pop() + while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): + do.append(self._pendingTimedCalls.pop()) + for call in do: try: call.called = 1 call.func(*call.args, **call.kw)
A few corrections: 1. I left the do=[] statment out of the patch. Its been added. (I may still have made a format error in this hand-crafted patch, so please take care!) 2. Most of the speed loss now is due to changes in my code for supporting thread migration. So speed is not an issue. Bill Bill la Forge <laforge49@yahoo.co.in> wrote: I'm not much good at patch files--lacking the tools. But take a look at the attached hack (crafted manually). It works for me, with luck it should pass your regression tests. (Its got the required 2 loops I was talking about.) I'll note in passing that I'm only getting a measure of 7K with this code. But that's probably as good as we can get without my patches. Good enough, I suspect. Bill Yahoo! India Matrimony: Find your partner online. Index: base.py =================================================================== --- base.py (revision 10658) +++ base.py (working copy) @@ -368,7 +368,11 @@ assert callable(_f), "%s is not callable" % _f assert sys.maxint >= _seconds >= 0, \ "%s is not greater than or equal to 0 seconds" % (_seconds,) - tple = DelayedCall(seconds() + _seconds, _f, args, kw, + if _seconds == 0: + tcc = 0 + else: + tcc = seconds() + _seconds + tple = DelayedCall(tcc, _f, args, kw, self._pendingTimedCalls.remove, self._resetCallLater) insort(self._pendingTimedCalls, tple) @@ -416,8 +420,11 @@ count += 1 del self.threadCallQueue[:count] now = seconds() - while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): - call = self._pendingTimedCalls.pop() + do=[] + while self._pendingTimedCalls and (self._pendingTimedCalls[-1].time <= now): + do.append(self._pendingTimedCalls.pop()) + for call in do: try: call.called = 1 call.func(*call.args, **call.kw)
participants (5)
-
Bill la Forge -
Christopher Armstrong -
Glyph Lefkowitz -
Itamar Shtull-Trauring -
Phillip J. Eby