[Pharo-project] About announcements
Henrik Sperre Johansen
henrik.s.johansen at veloxit.no
Mon Jan 2 09:50:12 EST 2012
On 02.01.2012 14:45, Igor Stasenko wrote:
> On 2 January 2012 10:42, Lukas Renggli<renggli at gmail.com> 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:
>>> 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.
Yup, leftover from live migration.
>> 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.
Yup, leftover from live migration/when whole delivery was in critical
section (ie before copying), a Semaphore should be ok. Can't remember
why we picked Monitor over Mutex in the first place :(
>> 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.
Alternative is the way the old code worked: Copy and replace collection
whenever you add/remove subscriptions.
Then there need be no critical section in deliver: at all.
Of course, it's a performance tradeoff between fast
subscription/unsubscription and delivery, one is not necessarily better
than the other.
Question is if it's worth providing/switching between different
SubscriptionRegistries based on actual use pattern?
Or, is a default generally better, and if so, which of the approaches
should be favored?
Lukas seem to thing delivery, and I'm inclined to agree with him,
I think we were a bit blinded by benching lots of
subscriptions/unsubscriptions in relations with the weak
implementations, and lost sight of the most common use pattern.
>> 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
With subs parameter a sequenceable collection, it could be made nearly
non-existant by iterating manually rather than through do:, moving
ifCurtailed to the outer scope. (resuming from index rather than a copy
after failed delivery).
Code would be uglier though.
>> 8. It then wraps the actual notification into an exception handler.
>> Again, this is rarely useful and should probably rather be something
> 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.
6 and 8. both arrived as responses to a desire to make delivery more robust.
I personally argued for using 6, but not 8, for a default announcer as a
Would not mind seeing this pluggable, and in either case moved to
SubscriptionRegistry to sit alongside the curtailing, so there is only
one class to modify for testing different delivery/error
>> 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.
Personally, it was to make it slightly more compatible with VW
announcements (which also pass the subscription as an optional 3rd param).
One use is when weak announcements don't give strong enough guarantees
on termination, and your block does not have access to the announcer.
(usually when an object is passively registered for listening)
A >> handlerBlock
[:announcement :announcer | self isValid
ifTrue: [self writeSomeStateUsedElseWhereBasedOn: announcement ]
ifFalse: [announcer unsubscribe: self]]
A listenTo: announcer
announcer when: IWantToListen do: self handlerBlock
Yes, you can also do the same by keeping an extra inst var with all
announcers A are sent as arguments, personally I find that less
desirable to maintain.
What I didn't notice at the time though, was that cull: makes
MessageSend subscription delivery quite slow, due to String numArgs
So that specific case needs optimization, otherwise the overhead of
supporting this is negligible.
More information about the Pharo-dev