On 02.01.2012 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.
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
