Ok I suggest that we wait for henrik feedback and turn these points in bug entry issue.
On Jan 2, 2012, at 2:45 PM, Igor Stasenko wrote: > On 2 January 2012 10:42, Lukas Renggli <[email protected]> wrote: >>>>> To you, the current Pharo image is the Pharo Core and you are unhappy >>>>> that it is too big (for example because RB is there). >>>> >>>> The size is the least problem. >>>> >>>> More annoying is that the code quickly gets out of sync and >>>> non-changes are added to the history. Both of these problems are >>>> already visible today. >>>> >>>> Additionally, over time people will change/add/remove features that >>>> get integrated without proper review. I just had a look at the >>>> announcement framework in Pharo 1.4 today, it is unbelievable how a >>>> tiny framework could degrade to a bloated, untested and dead slow pile >>>> of mud in just a few years :-( >>> >>> I think your are unfair here. The new features might be untested (current >>> coverage is at 56%), but the changes were meant to provide working weak >>> announcements. And they do work. >> >> Nice argument for your students reading this thread: >> http://memegenerator.net/instance/12779750. >> >>> But, what do you mean by slow? How did you benchmark it? >> >> No, but if you look at the code you will see many extra steps: >> >> 1. It tests if a registry is there: Why would that be necessary in an >> object-oriented implementation? >> > this should be cleaned up. > i think it is a leftover from migration code from old Announcer class (pre > 1.3) > to a new one. > We did things incrementally, and had code to migrate all instances > from old format to a new one, > without losing subscription and without stopping working. > >> 2. It tests if the registry is empty: Why would that be necessary in >> an object-oriented implementation? >> > not necessary. looks like a optimization. > >> 3. It enters a critical section of a monitor: This is rarely useful >> and the slowest kind of concurrency control available in Pharo (a >> Mutex would be enough for what it is used for, and instantiate >> multiple semaphores and queues), and btw the lazy initialization of >> the monitor is the prime example of a race condition. >> > agreed here. I would even leave a semaphore. > I think it is overlooked by Henrik. > It also don't needs a lazy initialization in #protected: , since in > #initialize it can just create a ready for use fresh > synchronization object. > >> 4. It creates a copy of the collection of all the subscriptions: This >> is rarely useful and wouldn't be necessary if an immutable data >> structure was used. >> > propose better solution how to deal with situation when during > handling an announcement, > your handler unsubscribing from announcer. > >> 5. It iterates over all announcements, it doesn't even try to group >> announcements of the same kind. >> > it is pointless to group them, and makes no real difference. > Because announcer doesn't knows what kinds of announcements will be > announcement, > and it knows only about subscriptions. > Suppose i have a subscription to Announcement. And i announcing > AnnouncementA (a subclass of it). > > Now it is still have to iterate over inheritance chain in order to > determine if given subscription should receive it or not. > Of course you can put all subscriptions to Announcement into one > group, so you don't need to check it for every subscription: > > dict at: Announcement put: group. > .... > so you can do: > > group do: [:subscription | subscripton deliver: announcement ]. > > but that imposing that you have a fixed model for announcement relationship, > while with #handles: i can simply make it so, that my announcement > class are not inherits from its superclass > and so, even if you subscribe to Announcement, you won't receive an > announcements of my kind, because > it simply doesn't walks an inheritance chain in its #handles: method. > > also, in the end i don't think it matters. Grouping is more optimal > (potentially), but you won't see any difference unless you > have hundreds of subscriptions, so i think it is not a big deal. > Usually there are few subscriptions held by announcer. And so there is > simply no need to bother and grouping them. > I cannot imagine an announcer holding hundreds or thousands of subscriptions. > > Announcer allSubInstances collect: [:e | e numberOfSubscriptions ] > an OrderedCollection(0 0 1 0) > >> 6. It wraps each announcement delivery into a curtailed block. It does >> that even for those that are never actually announced. >> > yeah, this can be optimized > >> 7. It then tests each announcement if it matches, again it doesn't >> even try to group the announcements of the same kind. Aside, the match >> test was changed so that it doesn't allow instance-based announcements >> anymore, breaking some of my code. >> > what is instance-based announcements? > > you can use any object as a subscription selector, just make sure it > understands #handles: message. > i.e. > > announcer on: myObject do: [ ... ] > where myObject can be any object, not just Announcement (sub)class. > > >> 8. It then wraps the actual notification into an exception handler. >> Again, this is rarely useful and should probably rather be something >> pluggable. >> > perhaps we can add even more 'bloat' to have 'safe' and 'unsafe' announcers :) > But for things like SystemAnnouncer, this is not a question that > exception handler must be present > and delivery must be guaranteed. > >> 9. It then culls the announcement and announcer into the block. Not >> sure why the announcer is useful, after all the block was registered >> with the announcer at some point. >> > cull there is for passing optional arguments. > >> So all these 9 steps are not really necessary (or even wanted) in most >> case. I doubt that any of them makes the code faster. I am glad that >> the code works for you even if all of this new functionality is >> untested. And good luck with the race condition :-) >> >>> There is a point in here. But, as I said, I thought that the point is to >>> produce the core by having jenkins strip away unwanted material. Of course, >>> the other way would be to start from the core as a seed and have jenkins >>> produce the current image. >> >> Do you really believe that stripping away unwanted material works? No >> other open source project in the whole world does it like that, >> everybody builds distributions on top of a smaller core: Linux, GCC, >> Gnome, FireFox, ... Even in the supermarket, you typically don't get a >> vegetable hamper if you just want a some potatoes. >> >> Lukas >> >> -- >> Lukas Renggli >> www.lukas-renggli.ch >> > > > > -- > Best regards, > Igor Stasenko. >
