[Cuis-dev] allSubclasses and withAllSubclasses improvement

[Juan Vuletich]

Uh, I hadn't seen you had already attached code, and only answered to 
the comments. Apologies for that!

This new version is much faster. Thanks! I integrated it at GitHub, with 
a short comment about the iterative vs. recursive choice.

Cheers,

-- 
Juan Vuletich
www.cuis-smalltalk.org
https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev
https://github.com/jvuletich
https://www.linkedin.com/in/juan-vuletich-75611b3
@JuanVuletich



On 8/23/2019 7:34 PM, Andres Valloud via Cuis-dev wrote:
> See attached for your reference.
>
> On 8/22/19 05:36, Juan Vuletich wrote:
>> On 8/21/2019 4:29 PM, Andres Valloud via Cuis-dev wrote:
>>> What do you see as "ugly"?
>>
>> It is way more complicated than needed to solve the problem. The 
>> problem is trivial, this code is not. And there is no reason for 
>> this: class hierarchy depths shouldn't be so deep to be a challenge 
>> for the call stack.
>>
>> Hernán's code is only as complex as the problem, without accidental 
>> complexity (in the Fred Brooks sense). It is a clear explanation of 
>> the problem and the solution.
>>
>> To me, code aesthetics is not only about code being clear, and a good 
>> explanation of the problem and the solution. It is also about being 
>> the best fit for the problem (including human readers here: the 
>> system is the curricula). Sometimes, performance requirements or 
>> space constraints require additional complexity. Then, I see that 
>> extra complexity as pretty. This is of course subjective, like any 
>> aesthetic judgment.
>>
>>> One detail I don't see mentioned here: the old code filled the 
>>> ordered collection breadth first (before the set conversion), and so 
>>> it has limited execution stack depth.  The current code fills the 
>>> collection depth first, so its execution stack depth is as deep as 
>>> the hierarchy tree.
>>>
>>> Maybe there was a reason why the users of allSubclasses were better 
>>> off with breadth first, before someone put in asSet.  Or perhaps 
>>> asSet was always there, I don't see why that was beneficial.
>>>
>>> In any case, I tried both the recursive and (properly implemented) 
>>> iterative methods: breadth first is >20% faster.
>>>
>>> On 8/21/19 05:44, Juan Vuletich via Cuis-dev wrote:
>>>> Wow, thanks. The old code was slow and ugly!
>>>>
>>>> Integrating it.
>>>>
>>>> Cheers,
>>>>
>>>> -- 
>>>> Juan Vuletich
>>>> www.cuis-smalltalk.org
>>>> https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev
>>>> https://github.com/jvuletich
>>>> https://www.linkedin.com/in/juan-vuletich-75611b3
>>>> @JuanVuletich
>>>>
>>>>
>>>> On 8/21/2019 9:26 AM, Hernan Wilkinson via Cuis-dev wrote:
>>>>> Hi,
>>>>>  attached is a cs that changes the implementation of 
>>>>> #allSubclasses and #withAllSubclasses and generates performance 
>>>>> improvement drastically.
>>>>>  Just as a quick example, with the current implementation:
>>>>>  Time millisecondsToRun: [ 100 timesRepeat: [Behavior 
>>>>> withAllSubclasses ]] 1140 .
>>>>>  With the proposed change:
>>>>>  Time millisecondsToRun: [ 100 timesRepeat: [Behavior 
>>>>> withAllSubclasses2 ]]  . 73
>>>>>
>>>>>  In the type checker of LiveTyping, checking Behavior goes from 
>>>>> 9.5 seconds to 5 seconds only with that change.
>>>>>  One difference between the current and proposed version, is that 
>>>>> the current returns a Set while the proposed version returns an 
>>>>> OrderedCollection.
>>>>>  I looked carefully if that could break something and it does not. 
>>>>> I've been using the image with this new implementation and 
>>>>> everything works fine.
>>>>>
>>>>>  Juan, please take a look at it and integrate it if you think it 
>>>>> is useful.
>>>>>  Hernan.
>>>>>
>>>>> -- 
>>>>> *Hernán Wilkinson
>>>>> Agile Software Development, Teaching & Coaching*
>>>>> *Phone: +54-011*-4893-2057
>>>>> *Twitter: @HernanWilkinson*
>>>>> *site: http://www.10Pines.com <http://www.10pines.com/>*
>>>>> Address: Alem 896, Floor 6, Buenos Aires, Argentina
>>>>
>>
>>