[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:
>> 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.
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
>> 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.

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 
reasonable(?) compromise.
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 
handling/robustness strategies.
>
>> 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 
implementation...
So that specific case needs optimization, otherwise the overhead of 
supporting this is negligible.

Cheers,
Henry




More information about the Pharo-dev mailing list