I know everyone is busy and working in their own domains not having the time to review a lot of other people's issues. However I am on my own, not part of a team where I can walk over and tap someone on the shoulder to ask for review of an issue I've resolved. It is a bit discouraging not to be able to proceed for lack of review. I'd be glad for any feedback including "its crap, do it a different way". So consider this a virtual should tap. I have... * refactored the two calls to #forMilliseconds: out to a single call * replaced call to #wait with #waitOr: * added #waitOr: Here is the diff... WorldState >> interCyclePause: milliSecs | currentTime wait | self serverMode ifFalse: [ (lastCycleTime notNil and: [CanSurrenderToOS ~~ false]) ifTrue: [ currentTime := Time millisecondClockValue. wait := lastCycleTime + milliSecs - currentTime. + "wait is the time remaining until milliSecs has passed + since lastCycle. If wait is zero or less, no need to Delay" + "If wait is greater than milliSeconds then bypass Delay, + by setting wait to zero." - (wait > 0 and: [ wait <= milliSecs ] ) ifTrue: [ - (Delay forMilliseconds: wait) wait ] ] ] + wait > milliSecs ifTrue: [ wait := 0 ] ] ] "btw, wait>milliSecs can only occur when clock rolls over. Eliminate after integrating issue 14353 ?" - ifTrue: [ (Delay forMilliseconds: 50) wait ]. + ifTrue: [ wait := 50 ]. + wait > 0 ifTrue: [ + (Delay forMilliseconds: wait) waitOr: [ + self inform: 'WorldState>>interCyclePause failed.' ] ]. lastCycleTime := Time millisecondClockValue. CanSurrenderToOS := true. "--------------------------" +Delay >> waitOr: anErrorBlock + self schedule. + [ beingWaitedOn + ifTrue: [ delaySemaphore wait ] + ifFalse:[ anErrorBlock value ] + ] ifCurtailed:[self unschedule]. +"Only the high priority timer event loop (TEL) signals /delaySempahore/. If the + TEL is not running, a delay will block forever (for example the UI will lock up). + /beingWaitedOn/ is only set to true from the TEL (via + DelayScheduler>>scheduleDelay: and Delay>>timingPriorityWaitedOn:) so test + this to determine if TEL is running. Avoid waiting when the TEL is not running." "--------------------------" This is for issue 14669 "Delay refactoring (part 2a) - avoid UI locking up when timer event loop is stopped" that is required to facilitate integration of issue 14353 "Delay refactoring (part 2) - change from milliseconds to microseconds. cheers -ben https://pharo.fogbugz.com/default.asp?14669 https://pharo.fogbugz.com/default.asp?14353
Ben Coman wrote:
I know everyone is busy and working in their own domains not having the time to review a lot of other people's issues. However I am on my own, not part of a team where I can walk over and tap someone on the shoulder to ask for review of an issue I've resolved. It is a bit discouraging not to be able to proceed for lack of review. I'd be glad for any feedback including "its crap, do it a different way". So consider this a virtual should tap.
I have... * refactored the two calls to #forMilliseconds: out to a single call * replaced call to #wait with #waitOr: * added #waitOr:
Here is the diff...
WorldState >> interCyclePause: milliSecs | currentTime wait | self serverMode ifFalse: [ (lastCycleTime notNil and: [CanSurrenderToOS ~~ false]) ifTrue: [ currentTime := Time millisecondClockValue. wait := lastCycleTime + milliSecs - currentTime.
+ "wait is the time remaining until milliSecs has passed + since lastCycle. If wait is zero or less, no need to Delay"
+ "If wait is greater than milliSeconds then bypass Delay, + by setting wait to zero." - (wait > 0 and: [ wait <= milliSecs ] ) ifTrue: [ - (Delay forMilliseconds: wait) wait ] ] ] + wait > milliSecs ifTrue: [ wait := 0 ] ] ] "btw, wait>milliSecs can only occur when clock rolls over. Eliminate after integrating issue 14353 ?"
- ifTrue: [ (Delay forMilliseconds: 50) wait ]. + ifTrue: [ wait := 50 ].
+ wait > 0 ifTrue: [ + (Delay forMilliseconds: wait) waitOr: [ + self inform: 'WorldState>>interCyclePause failed.' ] ].
lastCycleTime := Time millisecondClockValue. CanSurrenderToOS := true.
"--------------------------"
+Delay >> waitOr: anErrorBlock + self schedule. + [ beingWaitedOn + ifTrue: [ delaySemaphore wait ] + ifFalse:[ anErrorBlock value ] + ] ifCurtailed:[self unschedule].
+"Only the high priority timer event loop (TEL) signals /delaySempahore/. If the + TEL is not running, a delay will block forever (for example the UI will lock up). + /beingWaitedOn/ is only set to true from the TEL (via + DelayScheduler>>scheduleDelay: and Delay>>timingPriorityWaitedOn:) so test + this to determine if TEL is running. Avoid waiting when the TEL is not running."
"--------------------------"
This is for issue 14669 "Delay refactoring (part 2a) - avoid UI locking up when timer event loop is stopped" that is required to facilitate integration of issue 14353 "Delay refactoring (part 2) - change from milliseconds to microseconds.
cheers -ben
https://pharo.fogbugz.com/default.asp?14669 https://pharo.fogbugz.com/default.asp?14353
I should add, the test procedure would be: 1. Open Tools > Process Browser Observe it shows "(80) Delay Scheduling Process: DelayScheduler>>runTimerEventLoop" 2. Merge slice 14669 3. In Workspace do "Delay stopTimerEventLoop" Observe many informs stream up screen. Now the UI might be a bit slow bit is still responsive. Previously the UI would have locked up. This is the point of this change - having a chance to correct the fault. 4. Update the list in Tools > Process Browser. Observe process "(80...)" is gone. 5. In Workspace do "Delay startTimerEventLoop" Observe the informs have stopped. cheers -ben
Hi Ben, First off, thank you for looking into this, it is important, but it is also delicate and few people dare look at this stuff. I did the scenario you described in #40450 on Mac OS X and everything went as you described: although there were tons of notification messages, the UI remained usable though slow. I did some Zn tests afterwards, including save/restart of the image and did not see any problems. Sven
On 18 Jan 2015, at 10:22, Ben Coman <btc@openInWorld.com> wrote:
Ben Coman wrote:
I know everyone is busy and working in their own domains not having the time to review a lot of other people's issues. However I am on my own, not part of a team where I can walk over and tap someone on the shoulder to ask for review of an issue I've resolved. It is a bit discouraging not to be able to proceed for lack of review. I'd be glad for any feedback including "its crap, do it a different way". So consider this a virtual should tap. I have... * refactored the two calls to #forMilliseconds: out to a single call * replaced call to #wait with #waitOr: * added #waitOr: Here is the diff... WorldState >> interCyclePause: milliSecs | currentTime wait | self serverMode ifFalse: [ (lastCycleTime notNil and: [CanSurrenderToOS ~~ false]) ifTrue: [ currentTime := Time millisecondClockValue. wait := lastCycleTime + milliSecs - currentTime. + "wait is the time remaining until milliSecs has passed + since lastCycle. If wait is zero or less, no need to Delay" + "If wait is greater than milliSeconds then bypass Delay, + by setting wait to zero." - (wait > 0 and: [ wait <= milliSecs ] ) ifTrue: [ - (Delay forMilliseconds: wait) wait ] ] ] + wait > milliSecs ifTrue: [ wait := 0 ] ] ] "btw, wait>milliSecs can only occur when clock rolls over. Eliminate after integrating issue 14353 ?" - ifTrue: [ (Delay forMilliseconds: 50) wait ]. + ifTrue: [ wait := 50 ]. + wait > 0 ifTrue: [ + (Delay forMilliseconds: wait) waitOr: [ + self inform: 'WorldState>>interCyclePause failed.' ] ]. lastCycleTime := Time millisecondClockValue. CanSurrenderToOS := true. "--------------------------" +Delay >> waitOr: anErrorBlock + self schedule. + [ beingWaitedOn + ifTrue: [ delaySemaphore wait ] + ifFalse:[ anErrorBlock value ] + ] ifCurtailed:[self unschedule]. +"Only the high priority timer event loop (TEL) signals /delaySempahore/. If the + TEL is not running, a delay will block forever (for example the UI will lock up). + /beingWaitedOn/ is only set to true from the TEL (via + DelayScheduler>>scheduleDelay: and Delay>>timingPriorityWaitedOn:) so test + this to determine if TEL is running. Avoid waiting when the TEL is not running." "--------------------------" This is for issue 14669 "Delay refactoring (part 2a) - avoid UI locking up when timer event loop is stopped" that is required to facilitate integration of issue 14353 "Delay refactoring (part 2) - change from milliseconds to microseconds. cheers -ben https://pharo.fogbugz.com/default.asp?14669 https://pharo.fogbugz.com/default.asp?14353
I should add, the test procedure would be:
1. Open Tools > Process Browser Observe it shows "(80) Delay Scheduling Process: DelayScheduler>>runTimerEventLoop"
2. Merge slice 14669
3. In Workspace do "Delay stopTimerEventLoop" Observe many informs stream up screen. Now the UI might be a bit slow bit is still responsive. Previously the UI would have locked up. This is the point of this change - having a chance to correct the fault.
4. Update the list in Tools > Process Browser. Observe process "(80...)" is gone.
5. In Workspace do "Delay startTimerEventLoop" Observe the informs have stopped.
cheers -ben
On Sun, Jan 18, 2015 at 7:44 PM, Sven Van Caekenberghe <sven@stfx.eu> wrote:
Hi Ben,
First off, thank you for looking into this, it is important, but it is also delicate and few people dare look at this stuff.
This is why it would be good to give it plenty of time to settle in before Pharo 4 release.
I did the scenario you described in #40450 on Mac OS X and everything went as you described: although there were tons of notification messages, the UI remained usable though slow. I did some Zn tests afterwards, including save/restart of the image and did not see any problems.
Sven
Thanks for testing. cheers -ben
2015-01-18 18:35 GMT+01:00 Ben Coman <btc@openinworld.com>:
On Sun, Jan 18, 2015 at 7:44 PM, Sven Van Caekenberghe <sven@stfx.eu> wrote:
Hi Ben,
First off, thank you for looking into this, it is important, but it is also delicate and few people dare look at this stuff.
This is why it would be good to give it plenty of time to settle in before Pharo 4 release.
I did the scenario you described in #40450 on Mac OS X and everything went as you described: although there were tons of notification messages, the UI remained usable though slow. I did some Zn tests afterwards, including save/restart of the image and did not see any problems.
Sven
Thanks for testing. cheers -ben
What about this notifications 'WorldState>>interCyclePause failed.' They happen quite often in some situations, for example open Nautilus and search for a class name in the class search dialog. This inform always pops up if the wait time is small: [(Delay forMilliseconds: 5) waitOtherwise:[1 inform:'otherwise']] fork
Hi ben I'm really bad in concurrent programming. I forwarded your mail to igor because he is good in concurrent programming and told me that monday he will start to work on his paid project so may be he will see my mail. I want to thank you for your energy and I understand that you can be frustrated by the lack of feedback. What you are doing is important. This is why I want to write a couple of chapter on concurrent programming. Stef
I know everyone is busy and working in their own domains not having the time to review a lot of other people's issues. However I am on my own, not part of a team where I can walk over and tap someone on the shoulder to ask for review of an issue I've resolved. It is a bit discouraging not to be able to proceed for lack of review. I'd be glad for any feedback including "its crap, do it a different way". So consider this a virtual should tap.
I have... * refactored the two calls to #forMilliseconds: out to a single call * replaced call to #wait with #waitOr: * added #waitOr:
Here is the diff...
WorldState >> interCyclePause: milliSecs | currentTime wait | self serverMode ifFalse: [ (lastCycleTime notNil and: [CanSurrenderToOS ~~ false]) ifTrue: [ currentTime := Time millisecondClockValue. wait := lastCycleTime + milliSecs - currentTime.
+ "wait is the time remaining until milliSecs has passed + since lastCycle. If wait is zero or less, no need to Delay"
+ "If wait is greater than milliSeconds then bypass Delay, + by setting wait to zero." - (wait > 0 and: [ wait <= milliSecs ] ) ifTrue: [ - (Delay forMilliseconds: wait) wait ] ] ] + wait > milliSecs ifTrue: [ wait := 0 ] ] ] "btw, wait>milliSecs can only occur when clock rolls over. Eliminate after integrating issue 14353 ?"
- ifTrue: [ (Delay forMilliseconds: 50) wait ]. + ifTrue: [ wait := 50 ].
+ wait > 0 ifTrue: [ + (Delay forMilliseconds: wait) waitOr: [ + self inform: 'WorldState>>interCyclePause failed.' ] ].
lastCycleTime := Time millisecondClockValue. CanSurrenderToOS := true.
"--------------------------"
+Delay >> waitOr: anErrorBlock + self schedule. + [beingWaitedOn + ifTrue: [ delaySemaphore wait ] + ifFalse:[ anErrorBlock value ] + ]ifCurtailed:[self unschedule].
+"Only the high priority timer event loop (TEL) signals /delaySempahore/. If the + TEL is not running, a delay will block forever (for example the UI will lock up). + /beingWaitedOn/ is only set to true from the TEL (via + DelayScheduler>>scheduleDelay: and Delay>>timingPriorityWaitedOn:) so test + this to determine if TEL is running. Avoid waiting when the TEL is not running."
"--------------------------"
This is for issue 14669 "Delay refactoring (part 2a) - avoid UI locking up when timer event loop is stopped" that is required to facilitate integration of issue 14353 "Delay refactoring (part 2) - change from milliseconds to microseconds.
cheers -ben
https://pharo.fogbugz.com/default.asp?14669 https://pharo.fogbugz.com/default.asp?14353
participants (4)
-
Ben Coman -
Nicolai Hess -
stepharo -
Sven Van Caekenberghe