Certainly my mistake to name it cosmetic. Maybe I should have said
feature-neutral.
It was more a matter of speed indeed. Of style too maybe, but there it
will be harder to reach a consensus :)

Concerning speed, thanks for providing the tiny benchmarks Juan.
I would not even try a macro benchmark because the changes won't be
noticeable until Cog.
But you have to believe Eliot, that will be worth a massive change. In
Objectworks/Visualworks that were worth.

Of course, applying the change to all the code rather than identifying
and modifying only the bottleneck is a bit of extreme... extreme what
? extreme lazyness !
I started manually to have a chance to review some dusty parts of the
image and found:
- some interesting pieces I wouldn't imagine were in
- some code probably broken,
- parts which could be simplified,
- many, many code duplications !
But reviewing 1700 methods writing to outer temps is a bit too many
for a single person.
After 200 of them, I finally deployed Eliot's machinery and just
reviewed the result of it.
Maybe the change is a bit premature... But I'd like people to get used
to a style that can be simple elegant and fast (by chance it can be
both !).

Concerning refactorings, what strikes me is the level of code
duplication in Squeak. Now, I have not enough authority to slay code
that is not mine or decide which is the best implementor. I have to
ask first. To my eyes, the only referent authority in activity is
Andreas, thanks for your efforts.

Nicolas

2009/12/28 Juan Vuletich <juan at jvuletich.org>:
> Andreas Raab wrote:
>>
>> Levente Uzonyi wrote:
>>>
>>> On Sun, 27 Dec 2009, Andreas Raab wrote:
>>>
>>>> Nicolas Cellier wrote:
>>>>>
>>>>> We shall now take the habit of declaring temps in inner scope possible
>>>>> and avoid writing in outer temps if possible.
>>>>
>>>> Why? As long as auto-declare temps inserts temps at method level and
>>>> auto-remove of unused temps simply fails for block-level temps, this policy
>>>> would be annoying at best.
>>>
>>> Because the code is cleaner and faster.
>>
>> Cleaner? How so? Why are temps scattered throughout a method "cleaner"?
>> Faster? Which benchmark is improved? By how much exactly?
>>
>> Not that I'm arguing all temps should always be at method level but
>> neither do I think all temps at block-level should be considered the only
>> acceptable variant. There is a trade-off which comes from various ways of
>> using the tools (for example visibility of a temp during debugging) and the
>> writer of the method should be allowed to decide which temps to move to
>> block-level and which ones not. For example, there is absolutely no
>> difference between:
>>
>> mumble: arg
>> | foo bar baz |
>> 1 to: arg do:[:i| ...].
>>
>> and
>>
>> mumble: arg
>> 1 to: arg do:[:i| | foo bar baz | ...].
>>
>> (except that I find the latter slightly harder to understand) Just like
>> with block-formatting, some people prefer it one way, some people the other
>> and the one-size-fits-all approach of forcing one's own prejudices on
>> everyone else is *extremely* problematic from my point of view. There is
>> room for differences in writing code and just because they're different
>> doesn't mean one way is strictly better and one way is strictly worse.
>>
>> If there's an advantage for the compiler to have temps be block-local,
>> then let the compiler deal with it, not the user. If we decide that we
>> should encourage to define temps at the innermost block-scope then let's fix
>> the tools to support that properly.
>>
>> But rewriting hundreds of unrelated methods should always result in a
>> *measurable* benefit not just some "uhm, I guess it's ... cleaner? ... or
>> maybe ... faster? ...".
>>
>> Cheers,
>> - Andreas
>>
>
> Hi Folks,
>
> I did a little benchmarking with the attached code. I got:
>
> Block creation:
> Smalltalk garbageCollect. [ 1000000 timesRepeat: [ PlayingWithClosures
> exp01LocalTemp ]] timeToRun 852
> Smalltalk garbageCollect. [ 1000000 timesRepeat: [ PlayingWithClosures
> exp01RemoteTemp ]] timeToRun 938
> Smalltalk garbageCollect. [ 1000000 timesRepeat: [ PlayingWithClosures
> exp01RemoteTempWithAssignment ]] timeToRun 1051
> So, block creation is quite faster if local temps are used, especially if
> storing in them is needed.
>
> Block activation:
> Smalltalk garbageCollect. [ |b| b := PlayingWithClosures exp01LocalTemp.
> 1000000 timesRepeat: [ b value ]] timeToRun 838
> Smalltalk garbageCollect. [ |b| b := PlayingWithClosures exp01RemoteTemp.
> 1000000 timesRepeat: [ b value ]] timeToRun 785
> Smalltalk garbageCollect. [ |b| b := PlayingWithClosures
> exp01RemoteTempWithAssignment. 1000000 timesRepeat: [ b value ]] timeToRun
> 833
> There is no real difference in block activation. There might be a slight
> advantage when reusing pre calculated values stored in remote temps.
>
> Besides, at
> http://www.mirandabanda.org/cogblog/2008/11/14/mechanised-modifications-and-miscellaneous-measurements/
> Eliot says: "Since the VM will evolve into one that doesn’t create contexts
> unless absolutely necessary, if a block doesn’t need to reference its
> enclosing context it wll be faster to create, since the enclosing context
> will not have to be created as well. In other words, in a VM that optimizes
> contexts away the current closure design involves at least two allocations,
> one for the closure and one for the context. Blocks that don’t access their
> outer environment at all, ... so called "clean blocks" clearly don’t need to
> have a context instantiated, but my current design requires a context
> because the closure accesses the method through the context. " So, the
> difference en block creation performance might get larger.
>
> BTW, as expected, I didn't find any difference between local and remote
> temps when blocks are optimized by the compiler.
>
> So. Is this relevant? Are there blocks that are _created_ many times to make
> a difference? I don't think so.
>
> However, if a block is answered from a method, or it is stored somewhere for
> later use (for example, in SortedCollections), using "clean blocks" is safer
> and nicer. For example:
> | a block1 block2 result |
> a := 1.
> result := OrderedCollection new.
> block1 := [ a ].
> block2 := [ :p | a := p ].
> result add: block1 value.
> block2 value: 7.
> result add: block1 value.
> block2 value: 9.
> result add: block1 value.
> result
> Will answer an OrderedCollection(1 7 9). If both these blocks were evaluated
> from different places, the behavior could be hard to understand and debug.
> That's to me the main reason to prefer local temps: building "clean blocks".
>
> Cheers,
> Juan Vuletich
>
> 'From Squeak3.10.2 of 18 December 2009 [latest update: #8499] on 28 December
> 2009 at 11:38:09 am'!
> Object subclass: #PlayingWithClosures
>        instanceVariableNames: ''
>        classVariableNames: ''
>        poolDictionaries: ''
>        category: 'Playing with Closures'!
> !PlayingWithClosures commentStamp: 'jmv 12/28/2009 10:25' prior: 0!
> Just some scripts for learning about Closures!
>
>
> "-- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- "!
>
> PlayingWithClosures class
>        instanceVariableNames: ''!
>
> !PlayingWithClosures class methodsFor: 'experiments' stamp: 'jmv 12/28/2009
> 11:22'!
> exp01Argument
>        ^ [ :a |
>                a+1 ]! !
>
> !PlayingWithClosures class methodsFor: 'experiments' stamp: 'jmv 12/28/2009
> 11:11'!
> exp01LocalTemp
>
>        ^ [ | a |
>                a := 1.
>                a+1 ]! !
>
> !PlayingWithClosures class methodsFor: 'experiments' stamp: 'jmv 12/28/2009
> 11:13'!
> exp01RemoteTemp
>        | a |
>        a := 1.
>        ^ [
>                a+1 ]! !
>
> !PlayingWithClosures class methodsFor: 'experiments' stamp: 'jmv 12/28/2009
> 11:14'!
> exp01RemoteTempOptimized
>        | a |
>        a := 1.
>        ^1>0 ifTrue: [
>                a+1 ]! !
>
> !PlayingWithClosures class methodsFor: 'experiments' stamp: 'jmv 12/28/2009
> 11:13'!
> exp01RemoteTempOptimizedWithAssignment
>        | a |
>        ^1>0 ifTrue: [
>                a := 1.
>                a+1 ]! !
>
> !PlayingWithClosures class methodsFor: 'experiments' stamp: 'jmv 12/28/2009
> 11:29'!
> exp01RemoteTempWithAssignment
>        | a |
>        ^ [
>                a := 1.
>                a+1 ]! !
>
>
>
>