Cosmetic: move or remove a few temps inside closures

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Cosmetic: move or remove a few temps inside closures

Nicolas Cellier
Sorry for the high level of noise...

I profited by the low traffic to apply Eliot mechanised tool (
http://www.mirandabanda.org/cogblog/2008/11/14/mechanised-modifications-and-miscellaneous-measurements
)
+ own manual changes to remove as much writes as possible (in a short
time frame) to remote (external scope) temporaries from within a
closure.
This should fast up a little bit the image (probably not that much
until Cog is available).

We shall now take the habit of declaring temps in inner scope possible
and avoid writing in outer temps if possible.
By now, the cosmetic changes has at least a virtue of serving as examples...

Cheers

Nicolas

Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Andreas.Raab
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.

Cheers,
   - Andreas

Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Nicolas Cellier
2009/12/27 Andreas Raab <[hidden email]>:

> 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.
>
> Cheers,
>  - Andreas
>

Oh yes, tools are not up to date. That should be changed too.
Meanwhile, we can also avoid writing codes like

        | ans |
        [ans := self = anObject] ifError: [:aString :aReceiver | ^ false].
        ^ ans

and replace with:

        ^[self = anObject] ifError: [false]

I guess one outer temp and one non local return removed are worth a
change in Cog, and to my taste, the code is not less clear.

Nicolas

Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Levente Uzonyi-2
In reply to this post by Andreas.Raab
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. The auto-declare stuff should be
fixed, just like the auto-remove feature (this is really annoying) and of
course the decompiler which has issues with variables declared in inlined
blocks. For example: (EventSensor >> #eventTickler) decompile.


Levente

>
> Cheers,
>  - Andreas
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Andreas.Raab
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


Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Levente Uzonyi-2
On Sun, 27 Dec 2009, 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"?

You can clearly see the purpose of the variables if they are defined
where they are used. You don't have review all parts of large methods if
you want to know what is happening. Etc.

> Faster? Which benchmark is improved? By how much exactly?

I guess only by a tiny bit, I didn't bother measuring. Blocks
don't touch outer contexts, less local variables in the method, objects
can be GC'd sooner. Sounds like better performance.

>
> 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:

I agree, but I think pushing the temporaries into the innermost context
where they are used is useful. Noone is forcing anyone to do so.

>
> 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.

I agree, that's why I'm not rewriting all methods to fit my taste. :) I
know that everyone who cares formatting the methods prefers his/her own version.

I think about blocks as unnamed functions and the syntax reflects this
too. The unnamed version of:

foo: aFoo bar: aBar

is

: aFoo : aBar

Now people are lazy and skip the space:

[ :aFoo :aBar |

Following this idea, the temporaries can be formatted the same way in
blocks and methods:

mumble: arg
    1 to: arg do: [ :i |
       | foo bar baz |
       ...].

And it's readable (IMO).

>
> 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.
>

I think the tools should support defining variables in the innermost scope
and removing variables from inner scopes. Adding a preference for the
first may be useful too.


Levente

> 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
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Andreas.Raab
Levente Uzonyi wrote:

> On Sun, 27 Dec 2009, Andreas Raab wrote:
>> 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:
>
> I agree, but I think pushing the temporaries into the innermost context
> where they are used is useful. Noone is forcing anyone to do so.

A contraire. Pushing temps into the innermost context in all packages
*is* forcing them on everyone else. I won't ever argue about changes
like these if they serve a goal (i.e., some bit of work that you're
trying to achieve) but making changes specifically for "cosmetic"
reasons in hundreds of methods deserves at least a bit of agreement
whether these changes are being seen as worthwhile. Precisely because
individual preferences differ. I think we do need a bit of agreement in
the community before we start pushing such wide-ranging cosmetic changes.

Cheers,
   - Andreas


Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Igor Stasenko
In reply to this post by Levente Uzonyi-2
Polishing, may have some benefits, but unfortunately do not gives us a
long term benefits,
because a simpler code means smaller code, not good looking code :)

Nicolas, i don't want to discourage you. Your dedication is beyond
praises, but as to me, we have a lot of
code, which requires deep revision & ground-up rewriting and from this
point, spending a precious time with just polishing seems like a waste
to me.

However, if you like such kind of envolvement, i got an idea for you:
get rid of globals. :)
We are moving towards more modular system, and direct use of globals
(class names) instead of message sends
stands against it, because for example, if Kernel requires an service,
provided by package, which can be made optional, a direct use of its
classes should be discouraged. Instead all such uses should be
replaced by a message send, and leaving one gate point,
which will connect dependent package with one that provides given services.

Here's my list of most hated globals:
- Display
- Sensor
- World
- SourceFiles
- SystemChangeNotifier
- Utils
all of the above could be replaced by corresponding message sends i.e.
'self display'
so then, in case if we would need to get rid of that or extend the
model, we can easily change the semantics with minimal effort, in a
few places, instead of many.
For example, 'self display' could  be changed to answer a display,
local to particular morph & its world instance, and so, potentially we
could use many displays simultaneously.




--
Best regards,
Igor Stasenko AKA sig.

Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Andreas.Raab
Igor Stasenko wrote:
> Nicolas, i don't want to discourage you. Your dedication is beyond
> praises, but as to me, we have a lot of
> code, which requires deep revision & ground-up rewriting and from this
> point, spending a precious time with just polishing seems like a waste
> to me.

I can only second that. I find polishing to be mostly useful within a
subsystem when I have time to do final tweaks but most parts of the
system are not in that shape and actually require more drastic changes.

> However, if you like such kind of envolvement, i got an idea for you:
> get rid of globals. :)

Actually, it is more worthwhile to think about how the globals interact
than merely replace them. If you look at what David and I have done with
  Projects you get the idea - define an interface that provides a
natural bottleneck for the context of the operation and vector it
through this.

> Here's my list of most hated globals:
> - Display
> - Sensor
> - World
> - SourceFiles
> - SystemChangeNotifier
> - Utils

Most of these could (and probably should) be vectored through Project
current (i.e., Project current display/sensor/world/sources) and it
would make some excellent sense because much of it is already project
bound. That's the kind of refactoring that is both useful and (when
complete) quite rewarding as it allows us to unload things that were
previously unloadable.

Cheers,
   - Andreas


Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Igor Stasenko
2009/12/28 Andreas Raab <[hidden email]>:

> Igor Stasenko wrote:
>>
>> Nicolas, i don't want to discourage you. Your dedication is beyond
>> praises, but as to me, we have a lot of
>> code, which requires deep revision & ground-up rewriting and from this
>> point, spending a precious time with just polishing seems like a waste
>> to me.
>
> I can only second that. I find polishing to be mostly useful within a
> subsystem when I have time to do final tweaks but most parts of the system
> are not in that shape and actually require more drastic changes.
>
>> However, if you like such kind of envolvement, i got an idea for you:
>> get rid of globals. :)
>
> Actually, it is more worthwhile to think about how the globals interact than
> merely replace them. If you look at what David and I have done with
>  Projects you get the idea - define an interface that provides a natural
> bottleneck for the context of the operation and vector it through this.
>

Of course i didn't meant doing things blindly. Sure thing, each global
requires personal attention.
If we replace use of global by gating all sends to another global
using new protocol,
then still its worthwhile, since at the end we'll have:
 a) 1 less global, used wildly
 b) new protocol defines a role and rules under which some object(s)
should play. This is a most important aspect of any well-engineered
software: clear distinction of roles.

>> Here's my list of most hated globals:
>> - Display
>> - Sensor
>> - World
>> - SourceFiles
>> - SystemChangeNotifier
>> - Utils
>
> Most of these could (and probably should) be vectored through Project
> current (i.e., Project current display/sensor/world/sources) and it would
> make some excellent sense because much of it is already project bound.
> That's the kind of refactoring that is both useful and (when complete) quite
> rewarding as it allows us to unload things that were previously unloadable.
>
well, as i said, each of them require personal attention.
But, if we manage to get rid of some of them, i wouldn't cry in despair :)

> Cheers,
>  - Andreas
>
>
>



--
Best regards,
Igor Stasenko AKA sig.

Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Eliot Miranda-2
In reply to this post by Nicolas Cellier


On Sun, Dec 27, 2009 at 12:30 PM, Nicolas Cellier <[hidden email]> wrote:
Sorry for the high level of noise...

I profited by the low traffic to apply Eliot mechanised tool (
http://www.mirandabanda.org/cogblog/2008/11/14/mechanised-modifications-and-miscellaneous-measurements
)
+ own manual changes to remove as much writes as possible (in a short
time frame) to remote (external scope) temporaries from within a
closure.
This should fast up a little bit the image (probably not that much
until Cog is available).

We shall now take the habit of declaring temps in inner scope possible
and avoid writing in outer temps if possible.
By now, the cosmetic changes has at least a virtue of serving as examples...

This is not purely cosmetic.  Block-local temps are more efficient than method-level temps assigned inside blocks.  The latter are indirect temps and cost more to access and cause blocks that use them to cost more to create.


Cheers

Nicolas




Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Juan Vuletich-4
In reply to this post by Andreas.Raab
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 ]! !


Reply | Threaded
Open this post in threaded view
|

Re: Re: Cosmetic: move or remove a few temps inside closures

Nicolas Cellier
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 <[hidden email]>:

> 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 ]! !
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Andreas.Raab
Nicolas Cellier wrote:
> 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.

But Eliot will also tell you that I'm quite the unbeliever unless you
show me numbers, and by numbers I generally don't mean synthetic micro
benchmarks ;-)

> 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 !).

Sure, but if that's the goal then we really need better tool support.
It's the tools that decide how this is being used - if the browser will
automatically declare temps in innermost scope you'll have most new code
use that automatically. If the tools break (such as when removing temps
from block scope) you have pain and suffering.

> 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.

There are parts of the system where I gladly defer to you. If you (or
anyone else) feel that there's a part that you'd like to fix but don't
understand enough I think posting here is always the right approach -
there are plenty of people like Igor, David, Colin, Eliot, Levente,
Bert, Yoshiki, and of course you and me and lots of others who can give
guidance to any part of the system. Lay out your idea, let people give
feedback on it, implement it, let people review your solution, ship it.
Authority is something you gain by actively contributing, and you got
some :-)

Cheers,
   - Andreas


Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Josh Gargus
In reply to this post by Eliot Miranda-2

On Dec 27, 2009, at 7:50 PM, Eliot Miranda wrote:



On Sun, Dec 27, 2009 at 12:30 PM, Nicolas Cellier <[hidden email]> wrote:
Sorry for the high level of noise...

I profited by the low traffic to apply Eliot mechanised tool (
http://www.mirandabanda.org/cogblog/2008/11/14/mechanised-modifications-and-miscellaneous-measurements
)
+ own manual changes to remove as much writes as possible (in a short
time frame) to remote (external scope) temporaries from within a
closure.
This should fast up a little bit the image (probably not that much
until Cog is available).

We shall now take the habit of declaring temps in inner scope possible
and avoid writing in outer temps if possible.
By now, the cosmetic changes has at least a virtue of serving as examples...

This is not purely cosmetic.  Block-local temps are more efficient than method-level temps assigned inside blocks.  The latter are indirect temps and cost more to access and cause blocks that use them to cost more to create.


Playing devil's advocate, this is surely something that the compiler could optimize away.

(not actually advocating, just sayin')

Cheers,
Josh






Cheers

Nicolas






Reply | Threaded
Open this post in threaded view
|

Re: Cosmetic: move or remove a few temps inside closures

Juan Vuletich-4
Josh Gargus wrote:

>
> On Dec 27, 2009, at 7:50 PM, Eliot Miranda wrote:
>
>>
>>
>> On Sun, Dec 27, 2009 at 12:30 PM, Nicolas Cellier
>> <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Sorry for the high level of noise...
>>
>>     I profited by the low traffic to apply Eliot mechanised tool (
>>     http://www.mirandabanda.org/cogblog/2008/11/14/mechanised-modifications-and-miscellaneous-measurements
>>     )
>>     + own manual changes to remove as much writes as possible (in a short
>>     time frame) to remote (external scope) temporaries from within a
>>     closure.
>>     This should fast up a little bit the image (probably not that much
>>     until Cog is available).
>>
>>     We shall now take the habit of declaring temps in inner scope
>>     possible
>>     and avoid writing in outer temps if possible.
>>     By now, the cosmetic changes has at least a virtue of serving as
>>     examples...
>>
>>
>> This is not purely cosmetic.  Block-local temps are more efficient
>> than method-level temps assigned inside blocks.  The latter are
>> indirect temps and cost more to access and cause blocks that use them
>> to cost more to create.
>
>
> Playing devil's advocate, this is surely something that the compiler
> could optimize away.
>
> (not actually advocating, just sayin')
>
> Cheers,
> Josh

I don't think that would be a good thing. It is true that if the temp is
used only inside the block it doesn't matter. But that leads to sharing
temps between several blocks, and that has different semantics. The
compiler can know that (and act accordingly), but I think it is better
for the programmer to know it!

>>
>>     Cheers
>>
>>     Nicolas
>>

Cheers,
Juan Vuletich