> Thanks Lukas for the detailed points. These are great because they are to the > point and they lead to hands-on solutions. > > And, thanks Igor for picking it up. If Henrik would join the discussion we > would have all parties involved and action can just start. > > And it was so easy.
indeed constructivist feedback (even negative). We were working on event and it also seems that Monitor in announcement was causing some problems. Now I would really like to know if the Monitor implementation is good. Stef > > Cheers, > Doru > > > > On 2 Jan 2012, at 14:45, 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. >> > > -- > www.tudorgirba.com > > "Some battles are better lost than fought." > > > >
