Whitewash (was Re: [squeak-dev] The Trunk: EToys-tfel.249.mcz)
tim at rowledge.org
Fri Sep 23 18:51:52 UTC 2016
This is an excellent bit of Morphic code to dismantle because it illustrates several things that really ought to get improved.
> "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]]]
> [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 -
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 Rowledge; tim at rowledge.org; http://www.rowledge.org/tim
Useful random insult:- An early example of the Peter Principle.
More information about the Squeak-dev