Levente Uzonyi wrote:
> On Tue, 16 Feb 2010, Juan Vuletich wrote:
>
>> Levente Uzonyi wrote:
>>> Why is MessageTally using the class variable Timer instead of an 
>>> instance variable?
>>
>> I really don't know. Perhaps to make it clear that nested tallies are 
>> not supported? BTW, I've just looked in Squeak 1.1, and it uses the 
>> Timer variable the same way. Hey, I even opened Apple Smalltalk-80, 
>> and it is the same too :).
>>
>
> Thanks for the research, it's good to know the original design 
> decision. Too bad the class comment doesn't tell anything about it. We 
> should fix that.

yes.

>
>>> Levente
>>>
>>> P.S.:
>>> I added a new instance variable timer to MessageTally, replaced all 
>>> occurence of Timer to timer, removed the #ifNotNil: checks and 
>>> successfully profiled the tests. Is anything wrong with this approach?
>>
>> I just tried the same. It seems to work. But  it seems whoever wrote 
>> MessageTally knew what he was doing. Using an instance variable would 
>> have been the obvious choice. But they decided to use a class 
>> variable. Mhhh. I looked at the old books and found nothing. Looking 
>> at Squeak 1.1. and Smalltalk-80, I believe it was because at that 
>> time there was no #ensure: and they wanted to be sure to kill the 
>> process, as it won't stop if you got a walkback or pressed alt-.. So 
>> I guess your proposal is ok. But on the other hand, I see no value in 
>> supporting nested tallies, and your proposal would require some testing.
>>
>> So, if you're willing to see how it works, and perhaps revert it if 
>> some problem arises, I'd say go ahead with it.
>>
>
> I agree there's no value in nested tallies, MessageTally doesn't give 
> meaningful results even if timer is an instance variable in this case.
> And I think you're right, MessageTally should raise an error if a 
> timer process is already running, but the error should be raised when 
> the second process is about to be created, not when the first is about 
> to be terminated.

I guess that would be best. Although maybe we get some error for some 
old hung Timer process. If that ever happens, at least, it would call 
our attention...

>
> I realized that the timer process is terminated by Debugger >> 
> #process:controller:context: and not nested tallies when running all 
> the tests profiled, since no MessageTally is created directly by tests.
> So replacing Timer with an instance variable is not a good idea, since 
> the debugger has to stop the timer process somehow, but maybe 
> suspending/resuming is enough.

Oh! Horrible code! First thing to do is to change MessageTally>>#close 
to #closeTally, and fix senders. At least, that makes it easy to spot 
which the real senders are. I really hate "false polymorphism"! After 
that, creating a new instance only to send #close... Really awful. 
Changed that to a #terminateTimerProcess class message.

I'd say the only reason to kill the process is (again) that there was no 
#ensure: back then. It would be best not to do it at all. The Timer 
process should end or be terminated without such hacks.

If we can handle properly Timer termination, and ensure there is no way 
to leave it running, we could remove #closeTally and 
#terminateTimerProcess, and then make Timer an instance variable.

Cheers,
Juan Vuletich