2009/10/31 Stéphane Ducasse <stephane.ducasse@inria.fr>:
Thanks. I push the discussion to pharo since lot of people are interested.
Stef
Begin forwarded message:
From: Ralph Boland <rpboland@gmail.com> Date: October 29, 2009 11:43:04 PM GMT+01:00 To: Stéphane Ducasse <stephane.ducasse@inria.fr> Cc: Levente Uzonyi <leves@elte.hu> 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 dont have to duplicate. You can just derive the two classes from a common HashedCollection ancestor. http://lists.squeakfoundation.org/pipermail/squeak-dev/2007-June/117894.html Nicolas
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 Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project