isKindOf: in Morphic code...
Locked
12
12
 
|
Hi All, Hi Marcel,
when I see code like this, and there's a lot of it in Morphic, !Flaps class methodsFor: 'testing' stamp: 'mt 5/17/2016 14:17'! anyFlapsVisibleIn: aWorld aWorld submorphsDo: [:m | (m isKindOf: FlapTab) ifTrue: [^ true]]. ^ false! ! I think this is performance thrown on the floor (isKindOf: is awfully slow, especially in huge hierarchies like Morphic, and bad design, restricting one to a concrete class). And I think that Morph provides a perfect place to put an extension that doesn't pollute Object. So I would like to see anyFlapsVisibleIn: aWorld aWorld submorphsDo: [:m| m isFlapTab ifTrue: [^true]]. ^ false! ! with the emphasis on isFlapTab ;-) _,,,^..^,,,_ best, Eliot |
|
|
On 04.07.2016, at 22:44, Eliot Miranda <[hidden email]> wrote: > Hi All, Hi Marcel, > > when I see code like this, and there's a lot of it in Morphic, > > !Flaps class methodsFor: 'testing' stamp: 'mt 5/17/2016 14:17'! > anyFlapsVisibleIn: aWorld > > aWorld submorphsDo: [:m | > (m isKindOf: FlapTab) ifTrue: [^ true]]. > > ^ false! ! > > I think this is performance thrown on the floor (isKindOf: is awfully slow, especially in huge hierarchies like Morphic, and bad design, restricting one to a concrete class). And I think that Morph provides a perfect place to put an extension that doesn't pollute Object. So I would like to see > > anyFlapsVisibleIn: aWorld > > aWorld submorphsDo: > [:m| m isFlapTab ifTrue: [^true]]. > ^ false! ! > > with the emphasis on isFlapTab ;-) Well, class testing seems to be a Morphic pattern, given #findA: (alias #submorphOfClass:) Best regards -Tobias PS: Not advocating anything just reporting what I found |
|
|
> On 04-07-2016, at 1:59 PM, Tobias Pape <[hidden email]> wrote: > > > On 04.07.2016, at 22:44, Eliot Miranda <[hidden email]> wrote: > >> Hi All, Hi Marcel, >> >> when I see code like this, and there's a lot of it in Morphic, >> >> !Flaps class methodsFor: 'testing' stamp: 'mt 5/17/2016 14:17'! >> anyFlapsVisibleIn: aWorld >> >> aWorld submorphsDo: [:m | >> (m isKindOf: FlapTab) ifTrue: [^ true]]. >> >> ^ false! ! >> >> I think this is performance thrown on the floor (isKindOf: is awfully slow, especially in huge hierarchies like Morphic, and bad design, restricting one to a concrete class). And I think that Morph provides a perfect place to put an extension that doesn't pollute Object. So I would like to see >> >> anyFlapsVisibleIn: aWorld >> >> aWorld submorphsDo: >> [:m| m isFlapTab ifTrue: [^true]]. >> ^ false! ! >> >> with the emphasis on isFlapTab ;-) > > Well, class testing seems to be a Morphic pattern, given #findA: (alias #submorphOfClass:) It is, and it’s horrible. One of the major things I did to speed up Scratch was fixing hundreds of badly placed isKindOf: sends. Ideally one can find a better algorithm and avoid the issue completely in some places. Not quite so good is adding lots of isThingummy methods but at least that is faster! This is at least architecturally better since there are plenty of cases where simple class inheritance is not sufficient as a test because several disjoint classes provide a facility. You *really* don’t want to see multiple isKindOf’s in side an important loop! I suspect that there are a lot of places where we should *not* simply use the list of submorphs and scan it with isKindOf type tests but instead keep more precise track of relevant submorphs. tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim Strange OpCodes: SD: Self Destruct |
Re: isKindOf: in Morphic code...
|
|
In reply to this post by Tobias Pape
On Mon, Jul 4, 2016 at 1:59 PM, Tobias Pape <[hidden email]> wrote:
I don't care. It's *WRONG*. isKindOf: is a _bug_.
_,,,^..^,,,_ best, Eliot |
Re: isKindOf: in Morphic code...
|
Hey Eliot, no, it's not a bug. ;-) Although, there is room for improvement. #isFlapTab sounds good. Note that this is not even a critical piece of code because is only called if you open a system window, for example. Try [Flaps anyFlapsVisibleIn: ActiveWorld] bench. It's fast enough. A microsecond here, having around 30 windows open. So, you will not notice a delay. Still, you're right. We should reduce the usage of #isKindOf:. Not polluting Object means still polluting Morph. (Yeah, maybe we should use slower computers to program.) Checks we should consider to add or use more often if it is already there: #isCornerGripMorph #isProportionalSplitterMorph #isCompiledMethod #isColor #isInfiniteForm ... still, a quick look through the usage of #isKindOf: in Morphic code revealed no serious performance issues. We should refactor MenuMorph to not need #isKindOf: that often. This might also be true for other places. The pattern "isClass" is not always a good solution for many cases in the system. Best, Marcel |
Re: isKindOf: in Morphic code...
|
|
Okay, here is some more constructive feedback on this issue. The problem is that there are morphs in the world that do not want to be covered: flaps, docking bars, etc. So, it would be good to attach a property to that morph that says "please do not cover me". Then the RealEstateAgent just has to iterate of a world's submorphs once, check for that property and some bounds and we are fine. No need for #isKindOf:. No need for #isFlapTab. Right now, the algorithm is even broken because we assume that all flaps have the same size. For example, there cannot be a bigger icon, a smaller text, or whatever. They all have to have the same size. Kind of unnecessary limitation. This is the actual "bug". Best, Marcel |
|
|
Mercel,
On Mon, 2016-07-04 at 14:05 -0700, marcel.taeumel wrote: > > > > > <snip> > > > > > Hi All, Hi Marcel, > > > > > > > > > > when I see code like this, and there's a lot of it in > > > > > Morphic, > > > > > > > > > > !Flaps class methodsFor: 'testing' stamp: 'mt 5/17/2016 > > > > > 14:17'! > > > > > anyFlapsVisibleIn: aWorld > > > > > > > > > > aWorld submorphsDo: [:m | > > > > > (m isKindOf: FlapTab) ifTrue: [^ true]]. > > > > > > > > > > ^ false! ! > > > > > > > > > > I think this is performance thrown on the floor (isKindOf: is > > > > > awfully > > > > slow, especially in huge hierarchies like Morphic, and bad > > > > design, > > > > restricting one to a concrete class). And I think that Morph > > > > provides a > > > > perfect place to put an extension that doesn't pollute > > > > Object. So I > > > > would > > > > like to see > Okay, here is some more constructive feedback on this issue. > > The problem is that there are morphs in the world that do not want to > be > covered: flaps, docking bars, etc. So, it would be good to attach a > property > to that morph that says "please do not cover me". Then the > RealEstateAgent > just has to iterate of a world's submorphs once, check for that > property and > some bounds and we are fine. > > No need for #isKindOf:. > No need for #isFlapTab. > > Right now, the algorithm is even broken because we assume that all > flaps > have the same size. For example, there cannot be a bigger icon, a > smaller > text, or whatever. They all have to have the same size. Kind of > unnecessary > limitation. > > This is the actual "bug". > > Best, > Marcel > It's great that you've eliminated the usage in this specific example but you are not seeing the forest for the trees. Eliot's comment wasn't concerned with what was being tested or why it was being tested, but rather how it was being tested. #isKindOf: is testing what exact kind of thing something is, and its performance implications, rather than how the thing you are testing behaves. So if someone were to come along with MyBetterFlapTab in an alternate class hierarchy, the test as it current stands will not only perform sub-optimally, but it also won't behave as expected (i.e. if MyBetterFlapTab implements the FlapTab protocols it is reasonable to expect an instance of it to work wherever an instance of FlapTab does) I think what Eliot was asking was that wherever such tests are being performed, and for whatever reason, to not use #isKindOf: and use a protocol-based #is* approach instead that doesn't look at the concrete class. Re: the concern about polluting Object, Morph, or whatever... perhaps using a more generalized #is: approach could help. Or if it is such a specialized (i.e. one-off) test that an #is* or #is: check would not make sense, there is always #respondsTo: Thanks, Phil |
Re: isKindOf: in Morphic code...
|
|
In reply to this post by marcel.taeumel
On Mon, Jul 4, 2016 at 1:58 PM, marcel.taeumel <[hidden email]> wrote: Eliot Miranda-2 wrote Yes, isKindOf: *is* a bug, at least in the usage of determining supported messages. Smalltalk has ad-hoc polymorphism that allows any class to implement and set of messages, without having to inherit from a superclasss that defines that set of messages. isKindOf: breaks this property; it restricts the designer to using a concrete class, in violation of the language's flexibility. isKindOf: is therefore most definitely a bug. One might think there are valid uses when editing a class hierarchy programmatically. But for this there are messages understood by classes, not instances, such as includesBehavior:, inheritsFrom: etc. These are legitimate. But isKindOf: is not. It is a horrible hack. Although, there is room for improvement. #isFlapTab _,,,^..^,,,_ best, Eliot |
Re: isKindOf: in Morphic code...
|
|
@Phil: #isKindOf: is no better or worse than the #isClass-check. Maybe performance-wise, but arguably not in terms of architecture and code design. While #isClass might be a little closer to idiomatic Smalltalk code, it should never be used as an excuse for bad design. @all: I do see beauty in the pattern of asClass/isClass. For example, asSet/isSet, asOrderedCollection/isOrderedCollection. However, anytime a programmer tends to use a type-check and there is no "isClass" available, I would rather suggest him to use #isKindOf: for the moment instead of adding another two methods to encode the "isClass" in both base class and subclass. Always think twice before blowing up a class' interface. It impedes code readability. Especially if we are talking about a base class that is already that big, like Morph. Then, if there is really no other choice -- design-wise -- one has to consider performance. #isKindOf: is expensive. Okay. Then you can easily add the isClass-check. Fine. Sorry, but this is absolutely no black-white decision as Eliot and Phil proposed. There are many factors to consider. Semantics, code/interface readability, performance, idioms. In many cases -- if not all -- I suspect a design issue behind the use of #isKindOf: or #isClass. Still, I do like #respondsTo: because it is specific to a scenario/domain and leverages the dynamic capabilities of Smalltalk. For example, "model respondsTo: #foobar" as an extension point in graphical widgets. We should *not* batch-convert all uses of #isKindOf: to an #isClass check but use our Senders-Tools to hunt down all the design issues left in the system. Best, Marcel |
Re: isKindOf: in Morphic code...
|
|
One more thing: If you really consider to implement the "isClass" check in two unrelated branches of an inheritance tree, you arguably have a design issue. Just use #respondsTo:. For example, TranscriptStream should not implement #isModel but rather implement the Model interface as needed. #respondsTo: will work fine. Best, Marcel |
|
|
In reply to this post by marcel.taeumel
Marcel,
On Tue, 2016-07-05 at 00:42 -0700, marcel.taeumel wrote: > > > @Phil: #isKindOf: is no better or worse than the #isClass-check. > Maybe > performance-wise, but arguably not in terms of architecture and code > design. > While #isClass might be a little closer to idiomatic Smalltalk code, > it > should never be used as an excuse for bad design. > It's not an #isClass-check, it's an #isProtocolCollectionCompliant- check. It is worse both from a performance and an architecture standpoint to use #isKindOf:. I agree that no amount of calling the 'correct' method (if there are such things) makes up for bad design, but that's not the argument here. > @all: I do see beauty in the pattern of asClass/isClass. For example, > asSet/isSet, asOrderedCollection/isOrderedCollection. > > However, anytime a programmer tends to use a type-check and there is > no > "isClass" available, I would rather suggest him to use #isKindOf: for > the > moment instead of adding another two methods to encode the "isClass" > in both > base class and subclass. Always think twice before blowing up a > class' > interface. It impedes code readability. Especially if we are talking > about a > base class that is already that big, like Morph. Then, if there is > really no > other choice -- design-wise -- one has to consider performance. > #isKindOf: > is expensive. Okay. Then you can easily add the isClass-check. Fine. > > Sorry, but this is absolutely no black-white decision as Eliot and > Phil We'll have to agree to disagree on this point. Type checking is fundamentally doing it wrong IMO. > proposed. There are many factors to consider. Semantics, > code/interface > readability, performance, idioms. > > In many cases -- if not all -- I suspect a design issue behind the > use of > #isKindOf: or #isClass. Still, I do like #respondsTo: because it is > specific > to a scenario/domain and leverages the dynamic capabilities of > Smalltalk. > For example, "model respondsTo: #foobar" as an extension point in > graphical > widgets. > Don't conflate what these methods are named with the role they perform. While many of the #is* methods may appear to be class checks, if used properly they are actually checks for compliance with collections of protocols. (this would be easier to see if Smalltalk in general did a better job of formalizing protocols) I understand that many Smalltalkers don't look at them this way, but I would argue that they should. Here are a few examples of why the distinction matters: 1) Proxy objects. If I create a ProtoObject subclass as my proxy wrapper and pass it into code that does an #isKindOf: check, it will blow up even though it may be a proxy for exactly the kind of object being tested for. If it does an #is* check, that will get forwarded to the actual object and things will work as expected. Don't look at what I am, look at what I tell you I can do! 2) Parallel and/or alternate class hierarchies that are functionally equivalent or supersets. Let's say you have created an alternate hierarchy for Magnitude and its subclasses. If you implement the appropriate methods and respond true to #is* (i.e. whatever it is behaving like), why should (most) receivers of these objects need to know or care that my MyFractionLike objects aren't actually instances of Fraction, a subclass of it, or even in the Magnitude hierarchy? 3) A class that is close enough and wants to appear to be, but in reality is something very different from, the thing it responded true to for #is*. I have a number of things that do this in places in the class hierarchy that make the most sense from an implementation standpoint, but not necessarily from a taxonomy standpoint. There are even one or two places in the base Squeak class hierarchy where classes aren't necessarily where they 'should' be from a taxonomy standpoint... so be it. Testing for behavior, not class name or location in the hierarchy, is the way to go. Thanks, Phil > We should *not* batch-convert all uses of #isKindOf: to an #isClass > check > but use our Senders-Tools to hunt down all the design issues left in > the > system. > > Best, > Marcel > > |
Re: isKindOf: in Morphic code...
|
|
In reply to this post by marcel.taeumel
> One more thing: If you really consider to implement the "isClass" check in > two unrelated branches of an inheritance tree, you arguably have a design > issue. Just use #respondsTo:. For example, TranscriptStream should not > implement #isModel but rather implement the Model interface as needed. > #respondsTo: will work fine. How fast is #respondsTo:? Can it be used in a tight loop? Stef |
Re: isKindOf: in Morphic code...
|
|
In reply to this post by Phil B
Hi Phil, code lives. It should get improved if new requirements become obvious. #isKindOf: can be a valid (itermediate) step on this road of code evolution. You cannot account for any possible extension points of applications that might be created in the system. This is what refactoring is for. If an #isClass check is added, it will become part of the interface and callable from any application. If it remains an #isKindOf: check, it might still evolve towards not needing such a check at all. Removing an #isClass check later might break applications that already rely on it. So, like many parts in the system, using #isKindOf: can be seen as an intermediate step. Now, programmers should be encouraged to also go the next step and think about whether it should become an #isClass check or whether the check is necessary at all. Such a decision cannot be made by a computer progam / automatically. Programmers have to review the code, think about it, decide, and implement/refactor. Calling this phenomenon a "bug" does not feel right. It feels like a blame on some particular developers. I don't think it is. We only have so much time. Best, Marcel |
Re: isKindOf: in Morphic code...
|
|
> Calling this phenomenon a "bug" does not feel right. It feels like a blame
> on some particular developers. I don't think it is. +1 Stef |
Re: isKindOf: in Morphic code...
|
|
A lot of the isKindOf: sends is used to find specific submorphs. ButtonMorph>>label | s | s := ''. self allMorphsDo: [:m | (m isKindOf: StringMorph) ifTrue: [s := m contents]]. ^ s One could give names to the submorphs as they are initialized, all morphs understand 'name:' Would that be a solution ? Best, Karl On Tue, Jul 5, 2016 at 1:52 PM, Stéphane Rollandin <[hidden email]> wrote: Calling this phenomenon a "bug" does not feel right. It feels like a blame |
|
|
> On 05-07-2016, at 6:17 AM, karl ramberg <[hidden email]> wrote: > > A lot of the isKindOf: sends is used to find specific submorphs. > > ButtonMorph>>label > | s | > s := ''. > self allMorphsDo: [:m | (m isKindOf: StringMorph) ifTrue: [s := m contents]]. > ^ s > > One could give names to the submorphs as they are initialized, all morphs understand 'name:' > > Would that be a solution ? That’s close to what I was suggesting except that I don’t much like the idea of giving a morph a name; it would be more effective to have an instvar in the *owner* and properly track the owned morph that way. For example in a ButtonMorph I suggest we should have an instvar ‘label’ and when we set the label morph it should be put in that instvar as well as in the submorphs list. That way we can find it quickly, correctly, without the isKindOf: or even isTextMorph. And after all, both of those prevent a graphic label. tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim Useful Latin Phrases:- Oblitus sum perpolire clepsydras! = I forgot to polish the clocks! |
|
|
In reply to this post by marcel.taeumel
> On 05-07-2016, at 12:45 AM, marcel.taeumel <[hidden email]> wrote: > > One more thing: If you really consider to implement the "isClass" check in > two unrelated branches of an inheritance tree, you arguably have a design > issue. Just use #respondsTo:. For example, TranscriptStream should not > implement #isModel but rather implement the Model interface as needed. > #respondsTo: will work fine. I disagree. Not to checking for protocol but to using #respondsTo: - it’s even slower that isKindOf: ! It doesn’t simply climb the class tree checking the class name, it climbs the class tree checking the name of every method in the methodDictionary! The big issue is the design failure implied by the use of this sort of test; we ought to solve as many of those as possible as soon as possible. Swapping an isKindOf: for some isFoo usage is not ideal by any stretch of the imagination but it can at least solve some important performance problems and it at least allows for multiple classes in disjoint hierarchies to respond sensibly. A better choice of names will often help make life clearer - for example #canBeButtonMorphLabel would be much better than #isStringMorph. At least that hints at a protocol test rather class membership. We could, of course, implement a real protocol management & testing capability. At one time I thought that Traits might do that but they went moribund before ever really coming to life. tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim An elephant is a mouse with an operating system. |
|
|
In reply to this post by timrowledge
On 05.07.2016, at 18:46, tim Rowledge <[hidden email]> wrote: >> On 05-07-2016, at 6:17 AM, karl ramberg <[hidden email]> wrote: >> >> A lot of the isKindOf: sends is used to find specific submorphs. >> >> ButtonMorph>>label >> | s | >> s := ''. >> self allMorphsDo: [:m | (m isKindOf: StringMorph) ifTrue: [s := m contents]]. >> ^ s >> >> One could give names to the submorphs as they are initialized, all morphs understand 'name:' >> >> Would that be a solution ? > > That’s close to what I was suggesting except that I don’t much like the idea of giving a morph a name; it would be more effective to have an instvar in the *owner* and properly track the owned morph that way. For example in a ButtonMorph I suggest we should have an instvar ‘label’ and when we set the label morph it should be put in that instvar as well as in the submorphs list. That way we can find it quickly, correctly, without the isKindOf: or even isTextMorph. And after all, both of those prevent a graphic label. What If I drop some other StringMorph onto the Button morph and make it a submorph of it? It's simple morph composition and the new morph would just act as label. This is captured by the behavior above or some protocol-testing methods but not if ButtonMorph itself would manage what its label morph is. I know this sounds arcane, but Morphic is a very direct-manipulation environment and a lot of things are determined by structure rather than explicit management. I find that noteworthy, interesting, and a little bit intriguing. After all, "only" the development tools make little use of that. It's different in etoys as far as I can tell. If you make a development tool, its imho wise to maintain all the administrative stuff flying around, labels, lists, buttons, check boxes, the like. But in "general" Morphic, determination by looking at what your submorphs are or can do (sic!) seems fine. Best regards -Tobias :) > > > tim |
Re: isKindOf: in Morphic code...
|
|
In reply to this post by marcel.taeumel
On Tue, Jul 5, 2016 at 12:42 AM, marcel.taeumel <[hidden email]> wrote: Eliot Miranda-2 wrote That's simply incorrect, and misunderstands Smalltalk dynamic typing. And it is precisely in terms of architecture and design that it is worse (although in performance terms it is terrible, especially when disconfirming membership). Consider Random and ReadStream. Random inherits from Object, not PositionableStream, yet it provides a subset of the Stream interface via next, next: and next:into:. PositionableStream provides the layout of inst vars that used to be expected by the VM primitives for next, nextPut: and position that used to be required for performance but no longer are. So there is no practical benefit to have Random inherit from PositionableStream, but Random *is* a Stream. So were we to want to test for streamness we *could* implement isStream in both PostionableStream and Random to answer true and in Object to answer false, but we *could not* use aRandom isKindOf: PositionableStream. So as I stated before, isFoo supports ad-hoc polymorphism, allowing the designer to use inheritance for reuse, and freeing them from (mis) using it for "type". Whereas isKindOf: restricts designs to increasingly rigid and brittle class hierarchies, which prevent natural Smalltalk style by a broken convention. So Phil simply misunderstands the issue. I stand by my assertion, backed by 35 years of Smalltalk programming, that isKindOf: _is a bug_. @all: I do see beauty in the pattern of asClass/isClass. For example, isKindOf: is simply lazy. It's easy to implement the isFoo pattern, and it can be commented properly (for example, the comment can specify exactly which selectors the truth of the message implies). isKindOf: is quick and dirty, but potentially ambiguous. Precisely what API are we expecting the receiver to exhibit? Ugh. Sorry, but this is absolutely no black-white decision as Eliot and Phil Agreed. If isKindOf: can be removed by better design, especially by double dispatching then great. But in cases where isKindOf: is used exclusively on general instances of some class other than Object (such as Morph), or arguably this and nil as well, replacing isKindOf: invocations can be done without polluting Object, and can provide much better documentation and performance, as well as the key design flexibility it provides.
_,,,^..^,,,_ best, Eliot |
Re: isKindOf: in Morphic code...
|
|
In reply to this post by timrowledge
What a great discussion! Excellent arguments on all sides.
I agree with Marcel that we should not do wholesale conversion of isKindOf:'s -- they might be able to go away entirely someday by some factoring or cleanup. We should just convert them to standard #is* methods lazily, as tangible, real-world needs to do so arise. I do think its wrong to say an is* check represents a "design failure". Does using #isNil represent a design failure? No. There is nothing wrong with TSTTCPW. As Marcel said, code lives. Best, Chris On Tue, Jul 5, 2016 at 12:01 PM, tim Rowledge <[hidden email]> wrote: > >> On 05-07-2016, at 12:45 AM, marcel.taeumel <[hidden email]> wrote: >> >> One more thing: If you really consider to implement the "isClass" check in >> two unrelated branches of an inheritance tree, you arguably have a design >> issue. Just use #respondsTo:. For example, TranscriptStream should not >> implement #isModel but rather implement the Model interface as needed. >> #respondsTo: will work fine. > > I disagree. Not to checking for protocol but to using #respondsTo: - it’s even slower that isKindOf: ! It doesn’t simply climb the class tree checking the class name, it climbs the class tree checking the name of every method in the methodDictionary! > > The big issue is the design failure implied by the use of this sort of test; we ought to solve as many of those as possible as soon as possible. Swapping an isKindOf: for some isFoo usage is not ideal by any stretch of the imagination but it can at least solve some important performance problems and it at least allows for multiple classes in disjoint hierarchies to respond sensibly. A better choice of names will often help make life clearer - for example #canBeButtonMorphLabel would be much better than #isStringMorph. At least that hints at a protocol test rather class membership. > > We could, of course, implement a real protocol management & testing capability. At one time I thought that Traits might do that but they went moribund before ever really coming to life. > > > tim > -- > tim Rowledge; [hidden email]; http://www.rowledge.org/tim > An elephant is a mouse with an operating system. > > > |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |