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 <[email protected]> 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 >
