Smalltalk › Squeak › Squeak - Dev

Whitewash (was Re: [squeak-dev] The Trunk: EToys-tfel.249.mcz)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

2 messages Options

Eliot Miranda-2

Whitewash (was Re: [squeak-dev] The Trunk: EToys-tfel.249.mcz)

Hi All,

in reading Tobias' much improved code below I noticed that a lot of code is using Morph>>allMorphs where Morph>>allMorphsDo: would be much better

On Fri, Sep 23, 2016 at 6:26 AM, <[hidden email]> wrote:

Tim Felgentreff uploaded a new version of EToys to project The Trunk:
http://source.squeak.org/trunk/EToys-tfel.249.mcz

==================== Summary ====================

Name: EToys-tfel.249
Author: tfel
Time: 23 September 2016, 3:26:09.056418 pm
UUID: 844e5444-0afb-144b-a622-dcb33fbf8597
Ancestors: EToys-tfel.248

fix the defensive code for updating status morphs in scripts

=============== Diff against EToys-tfel.248 ===============

Item was changed:
----- Method: ScriptInstantiation>>updateAllStatusMorphs (in category 'status control') -----
updateAllStatusMorphs
"Update all status morphs bound to the receiver. Done with a sledge-hammer at present."

| w |
w := self currentWorld.
+ (w hasProperty: #updateStatusMorph) ifTrue: [^ self].
+ w setProperty: #updateStatusMorph toValue: true.
+ Project current addDeferredUIMessage: [
+ (w valueOfProperty: #updateStatusMorph ifAbsent: [false]) ifTrue: [
+ (w allMorphs select: [:m | (m isKindOf: ScriptStatusControl) and:
+ [m scriptInstantiation == self]]) do:
+ [:aStatusControl | self updateStatusMorph: aStatusControl].
+ w removeProperty: #updateStatusMorph.
-
- (w hasProperty: #foo) ifFalse: [
- w setProperty: #updateStatusMorph toValue: true.
- Project current addDeferredUIMessage: [
- (w hasProperty: #foo) ifTrue: [
- w removeProperty: #updateStatusMorph.
- (w allMorphs select: [:m | (m isKindOf: ScriptStatusControl) and:
- [m scriptInstantiation == self]]) do:
- [:aStatusControl | self updateStatusMorph: aStatusControl] .
- ]
]
]

!

So the above could be

updateAllStatusMorphs

"Update all status morphs bound to the receiver. Done with a sledge-hammer at present."

| w |

w := self currentWorld.

(w hasProperty: self) ifTrue: [^ self].

w setProperty: self toValue: #updating.

Project current addDeferredUIMessage:

[[w allMorphsDo:

[:m |

((m isKindOf: ScriptStatusControl)

and: [m scriptInstantiation == self]) ifTrue:

[self updateStatusMorph: m]]]

ensure:

[w removeProperty: self]]

There are quite a few examples like this, even one egregious "ow allMorphs do: [:m|..." instead of "ow allMorphsDo: [:m|...". Calling for volunteers who know Morphic well to go through and make the necessary changes. This should help slow machines like RasperryPi a bit.

_,,,^..^,,,_

best, Eliot

timrowledge

Re: Whitewash (was Re: [squeak-dev] The Trunk: EToys-tfel.249.mcz)

This is an excellent bit of Morphic code to dismantle because it illustrates several things that really ought to get improved.

>
> updateAllStatusMorphs
> "Update all status morphs bound to the receiver. Done with a sledge-hammer at present."
>
> | w |
> w := self currentWorld.
> (w hasProperty: self) ifTrue: [^ self].
> w setProperty: self toValue: #updating.
> Project current addDeferredUIMessage:
> [[w allMorphsDo:
> [:m |
> ((m isKindOf: ScriptStatusControl)
> and: [m scriptInstantiation == self]) ifTrue:
> [self updateStatusMorph: m]]]
> ensure:
> [w removeProperty: self]]
>
> There are quite a few examples like this, even one egregious "ow allMorphs do: [:m|..." instead of "ow allMorphsDo: [:m|...". Calling for volunteers who know Morphic well to go through and make the necessary changes. This should help slow machines like RasperryPi a bit.

Trying to work ‘outwards’ -

- breaking of encapsulation in 'm scriptInstantiation == self]) ifTrue: [self updateStatusMorph: m]]]’
This is a common pattern I really dislike.
Lots of ‘(a getaninstvar comparewith: b) ifTrue:[a setinstvar: b getsomeinstvar]’. Ought to be more like ‘a doSomethingWith: b’. In the above case, how about ‘m updateStatusRelatingTo: self’

- use of isKindOf:
A wrist-slapping offence at the very least. It’s slow. It’s just plain *wrong* in many cases since it ignores functionality in favour of fake-type-name. Even worse, it encourages people to make not-very-related things subclasses of some inappropriate parent class just to make some nasty code function.
As a nasty but at least improving hack, #isStatusIndicatingMorph would be better. Yes, it means another silly looking ^false method in Morph, but at least the relevant subclasses don’t have to be constrained into a single inheritance tree. And with caching and cogging the size of method dictionaries is long past being an issue to worry about. If you don’t like seeing lots of little near-null methods, then improve the browsers to help hide them!
So we could now make the inner code more like -
‘w allMorphsDo:[:m|
m isStatusIndicatingMorph ifTrue:[m updateStatusRelatingTo: self]]’
which is a bit less awful and potentially a fair bit faster.

- misuse of generality
Perhaps not the best description of the problem, but I’ll use it for now.
Simply because a morph has a list of submorphs into which all included morphs are supposed to go (though I’m reasonably sure there are one or two morph classes where this is broken) does not mean that we have to *only* put them there and always search every submorph with the concomitant misuse of #isKindOf: etc. It can be much better to add a status indicating morph (for example) via an #addStatusIndicatorMorph: method that adds the morph to both a list of status morphs and the main submorphs list. It also avoids any use of the #is… messages outside of maybe checking that the morph ought actually be accepted here..
Thus our loop becomes
‘w allStatusIndicatorMorphsDo: [:m| updateStatusRelatingTo: self]’
which could easily be dozens of times faster.

I’ve found hundreds of such cases in the old Scratch code and fixed many dozens, with a lot still to go. Such improvements account for something like an X8 increase in the user visible performance.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Useful random insult:- An early example of the Peter Principle.