Odd code in CwAppContext>updateCounter

updateCounter

"Update our counter to reflect the correct counter time

in milliseconds. This is guaranteed to work so long as

it is called once every 24 hours."

| timeMSV |

(timeMSV := Time millisecondClockValue) < lastCTime

ifTrue: [

  "The day has changed."

cTime := (cTime + (86400000 - lastCTime + timeMSV)) ]

ifFalse: [

"Add the difference in time to our counter."

cTime := cTime + (timeMSV - lastCTime) ].

 lastCTime := timeMSV

cTime := (cTime + (86400000 - lastCTime + timeMSV))

 cTime := (cTime + (-lastCTime + timeMSV))

Hi Guys,

I'm about to do something that I'm not really comfortable with but that I think I have to.  That is to criticize someone else's code.  If I'm wrong, tell me and I will take it all back.

While looking into my 100% CPU problem, I started looking into reasons my code would continue to run but the CPU would go to 100%.  This led me to see if I could find any reason for the idle loop not running or it not going to the OS.  That brought me to CwAppContext>updateCounter.  As far as I can tell CwAppContext>updateCounter is updating an instance variable called #cTime that is used as a base for a time a callback will be run.  The request to schedule a callback includes a time interval supposedly some number of milliseconds from now.  In reality it is some milliseconds from #cTime.

 

I don't really see a need for the calculated value #cTime (should just use Time millisecondClockValue) and therefor CwAppContext>updateCounter that calculates #cTime.  Lets put that aside for a moment and look at CwAppContext>updateCounter, which I think is buggy.

updateCounter

"Update our counter to reflect the correct counter time

in milliseconds. This is guaranteed to work so long as

it is called once every 24 hours."

| timeMSV |

(timeMSV := Time millisecondClockValue) < lastCTime

ifTrue: [

  "The day has changed."

cTime := (cTime + (86400000 - lastCTime + timeMSV)) ]

ifFalse: [

"Add the difference in time to our counter."

cTime := cTime + (timeMSV - lastCTime) ].

 lastCTime := timeMSV

This code uses Time millisecondClockValue, which it thinks is the number of milliseconds from midnight, it is not.  It is the time in milliseconds from when windows was booted.  The code thinks millisecondClockValue will role over to zero every 24 hours (86400000 milliseconds - maybe someone was thinking of Time asMilliseconds).  Actually it roles over about every 49 days.  It is trying to reset #cTime to a low value when the clock roles over.  The test for the role over looks good but the code to fix #cTime after a role over is not.  It seems to me the new value of #cTime will be 86400000 milliseconds too big.  This means that the next callback scheduled will be set for one day in the future.

cTime := (cTime + (86400000 - lastCTime + timeMSV))

should be:  

 cTime := (cTime + (-lastCTime + timeMSV))

but if you look at it, it is the same as the false path.  So the whole method can be reduced to:

updateCounter

"Update our counter to reflect the correct counter time

in milliseconds. This is guaranteed to work so long as

  it is called once every 49 days."

| timeMSV |

 timeMSV := Time millisecondClockValue.

 cTime := cTime + timeMSV - lastCTime.

 lastCTime := timeMSV

Now back to the need for the method.  A CwAppContext singleton keeps a sorted collection of callbacks that it runs when #idle is called.  The collection is sorted by a time calculated from the latest #cTime and the interval (more or less milliseconds from now) specified for the call back.  It looks like the #cTime values exists to deal with the role over of millisecondClockValue which is thought to be every 24 hours but is really very 49 days.  As far as I can tell there isn't any code that runs the list of callbacks and fixes them when the clock roles over.  This means any existing callbacks at the time of a role over will not be posted for another 49 days.  So, having #cTime and maintaining it is of no real value.

I would just use Time millisecondClockValue plus the interval time as the time to run the callback.  I would test for the role over (probably in #idle) and then fix the callbacks in the list.  No need for #cTime and the whole updateCounter method.

Since the role over is every 49 days and not every day and since the callback list is probably empty at the time of the role over, we may seldom see any problem from this code and as far as I can tell it has nothing to do with my 100% CPU problem.  But it should still be fixed or someone could please tell me I've got it all wrong.

Lou