Fwd: Adding FasterSets to Pharo: update

> From: Ralph Boland <[hidden email]>
> Date: October 29, 2009 11:43:04 PM GMT+01:00
> To: Stéphane Ducasse <[hidden email]>
> Cc: Levente Uzonyi <[hidden email]>
> Subject: Adding FasterSets to Pharo: update
>
> I took a look at FasterSets and Levente Uzonyi's version.
> I compared the two versions as of the first release of Levente's
> version to Squeak.  Note that he has released additional
> changes since then.
> Here is my report.
>
> 1)  Bugs:
>
>    FasterSets has two problems:
>      a)  KeyedIdentitySet is missing method  ScanForNil:
>      b)  PluggableSet is missing method  NoCompareOrGrowAddAll:
>    Both cases should cause failure.  Note that I have been using
>    this code for years without problem.  I assume that is because  
> the subset
>    of Squeak that I used doesn't include using these classes.
>    These problems are easily fixed.
>
>     In Levente's code class WeakKeyToCollectionDictionary->rehash  
> seems to be
>     missing.   This appears to be a mistate to me but I will live it
> to Levente to explain
>     it.  The fix, if needed, is simple.
>
> 2) Code quality:
>    For me the code quality is about the same but Levente considers
> his version to be better.
>    In any case changes in one version is easily ported to the other
> so it would be easy to
>    perform any cleanup you might want.  Three notes in particular.
>      a)  Levente's version of  rehash is cleaner and should be used
> instead of mine.
>      b)  Levente's version does not use method  scanForNilFrom:  (He
> would probably call
>           the method scanForEmptySlotFrom:).
>           I don't know why he didn't do this: it clearly cleaner.
>
>   3 Performance.
>       a) According to Levente's stats  rehash is clearly faster in
> his version than mine.  I see
>           no reason no not prefer his version to mine.
>
>       b) The improvement in performance for method add:  in  
> Levente's version
>           is insignificant   (appx 1%);  this difference could easily
> be made up in minor
>           changes to my code.  Thus here I consider there to be no  
> difference.
>
>      b)  Levente's version does not apply the FasterSets idea to
> MethodDictionary.
>           I am not sure why he did not do this but it may have
> something to do with the fact that
>           mistakes can easily corrupt your image.  The version in
> FasterSets works fine though
>           it took three tries for me to get it right (mostly because
> of carelessness).
>
>      c)  I left methods  intersection and nastyAddNew: out of classes
> Set and Dictionary for
>            the initial version of FasterSets to keep the number of
> changes to a minimum.
>            At your request I added them to the current version.
> These methods are not in
>            Levente's version.  They are public methods that provide
> performance improvements.
>            If desired they could easily be added to Levente's version.
>
>
> Based on the above I would  say that there is not a great deal of
> difference between Levente's version and mine but what difference
> there is favors his version.
>
> I recommend that
>
>     1)  You use Levente's version.
>     2)  Sort out the issue of no rehash method for Class
> WeakKeyToCollectionDictionary.
>     3)  Modify his code to use method scanForNilFrom:  (calling it
> scanForEmptySlotFrom:).
>     4)  Decide if you want to add methods  intersect: and  
> nastyAddNew:.
>          I can add these if you want.
>     5) Release a version of Pharo with these changes.
>     6)  In the following release of Pharo add the changes to
> MethodDictionary to use The
>          FasterSets Idea.  I sugggest doing this separately from
> everything else just because it is a
>          bit hairy.  I would not leave this change out because
> MethodDictionary is too important
>          a class not to have these improvements.
>
>
> One final comment.  If you really want to make the Set hierarchy
> better I would suggest that
> you move the Dictionary hierarchy out of the Set hierarchy
> altogether.  This means some
> duplication of code but makes for a much cleaner implementation.  For
> example you get to
> avoid the mess with MethodDictionary.   Of the three implementations
> of Smalltalk that I
> have looked at  Squeak/Pharo is the only version that uses one  
> hierarchy.
> This is probably too major a change to make but at least you now know
> my opinion on the
>  matter.
>
>
> You can post this on the Pharo newsgroup if you like but perhaps you
> should get Levente's
> opinion first.
>
>  Regards
>
>   Ralph Boland

> Thanks. I push the discussion to pharo since lot of people are
> interested.
>
> Stef
>
> Begin forwarded message:
>
>> From: Ralph Boland <[hidden email]>
>> Date: October 29, 2009 11:43:04 PM GMT+01:00
>> To: Stéphane Ducasse <[hidden email]>
>> Cc: Levente Uzonyi <[hidden email]>
>> Subject: Adding FasterSets to Pharo: update
>>
>> I took a look at FasterSets and Levente Uzonyi's version.
>> I compared the two versions as of the first release of Levente's
>> version to Squeak.  Note that he has released additional
>> changes since then.
>> Here is my report.
>>
>> 1)  Bugs:
>>
>>    FasterSets has two problems:
>>      a)  KeyedIdentitySet is missing method  ScanForNil:
>>      b)  PluggableSet is missing method  NoCompareOrGrowAddAll:
>>    Both cases should cause failure.  Note that I have been using
>>    this code for years without problem.  I assume that is because
>> the subset
>>    of Squeak that I used doesn't include using these classes.
>>    These problems are easily fixed.
>>
>>     In Levente's code class WeakKeyToCollectionDictionary->rehash
>> seems to be
>>     missing.   This appears to be a mistate to me but I will live it
>> to Levente to explain
>>     it.  The fix, if needed, is simple.
>>
>> 2) Code quality:
>>    For me the code quality is about the same but Levente considers
>> his version to be better.
>>    In any case changes in one version is easily ported to the other
>> so it would be easy to
>>    perform any cleanup you might want.  Three notes in particular.
>>      a)  Levente's version of  rehash is cleaner and should be used
>> instead of mine.
>>      b)  Levente's version does not use method  scanForNilFrom:  (He
>> would probably call
>>           the method scanForEmptySlotFrom:).
>>           I don't know why he didn't do this: it clearly cleaner.
>>
>>   3 Performance.
>>       a) According to Levente's stats  rehash is clearly faster in
>> his version than mine.  I see
>>           no reason no not prefer his version to mine.
>>
>>       b) The improvement in performance for method add:  in
>> Levente's version
>>           is insignificant   (appx 1%);  this difference could easily
>> be made up in minor
>>           changes to my code.  Thus here I consider there to be no
>> difference.
>>
>>      b)  Levente's version does not apply the FasterSets idea to
>> MethodDictionary.
>>           I am not sure why he did not do this but it may have
>> something to do with the fact that
>>           mistakes can easily corrupt your image.  The version in
>> FasterSets works fine though
>>           it took three tries for me to get it right (mostly because
>> of carelessness).
>>
>>      c)  I left methods  intersection and nastyAddNew: out of classes
>> Set and Dictionary for
>>            the initial version of FasterSets to keep the number of
>> changes to a minimum.
>>            At your request I added them to the current version.
>> These methods are not in
>>            Levente's version.  They are public methods that provide
>> performance improvements.
>>            If desired they could easily be added to Levente's version.
>>
>>
>> Based on the above I would  say that there is not a great deal of
>> difference between Levente's version and mine but what difference
>> there is favors his version.
>>
>> I recommend that
>>
>>     1)  You use Levente's version.
>>     2)  Sort out the issue of no rehash method for Class
>> WeakKeyToCollectionDictionary.
>>     3)  Modify his code to use method scanForNilFrom:  (calling it
>> scanForEmptySlotFrom:).
>>     4)  Decide if you want to add methods  intersect: and
>> nastyAddNew:.
>>          I can add these if you want.
>>     5) Release a version of Pharo with these changes.
>>     6)  In the following release of Pharo add the changes to
>> MethodDictionary to use The
>>          FasterSets Idea.  I sugggest doing this separately from
>> everything else just because it is a
>>          bit hairy.  I would not leave this change out because
>> MethodDictionary is too important
>>          a class not to have these improvements.
>>
>>
>> One final comment.  If you really want to make the Set hierarchy
>> better I would suggest that
>> you move the Dictionary hierarchy out of the Set hierarchy
>> altogether.  This means some
>> duplication of code but makes for a much cleaner implementation.  For
>> example you get to
>> avoid the mess with MethodDictionary.   Of the three implementations
>> of Smalltalk that I
>> have looked at  Squeak/Pharo is the only version that uses one
>> hierarchy.
>> This is probably too major a change to make but at least you now know
>> my opinion on the
>>  matter.
>>

>>
>> You can post this on the Pharo newsgroup if you like but perhaps you
>> should get Levente's
>> opinion first.
>>
>>  Regards
>>
>>   Ralph Boland
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>