Thanks. I push the discussion to pharo since lot of people are interested.
Stef Begin forwarded message: > From: Ralph Boland <[email protected]> > Date: October 29, 2009 11:43:04 PM GMT+01:00 > To: Stéphane Ducasse <[email protected]> > Cc: Levente Uzonyi <[email protected]> > 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 [email protected] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
