Hi Henrik,
Le 02/07/2015 01:57, Henrik Sperre Johansen a écrit :
Thierry Goubier wrote
Le 01/07/2015 21:29, Sven Van Caekenberghe a écrit :
On 01 Jul 2015, at 21:23, Thierry Goubier <
thierry.goubier@
> wrote:
Le 01/07/2015 21:11, Sven Van Caekenberghe a écrit :
On 01 Jul 2015, at 21:07, Thierry Goubier
<
thierry.goubier@
> wrote:
Le 01/07/2015 20:49, Sven Van Caekenberghe a écrit :
Reading the code from SubscriptionRegistry>>#deliver: is seems to
me that *on each announcement* a new array is created with the
subscriptions that want to handle the announcement. That strikes
me as pretty inefficient.
Maybe, there is also the comment
"using a copy, so subscribers can unsubscribe from announcer "
Oh, unsubscribe in the code processing an announcement?
but the price to pay for this seems to high to me.
And I'm not entirely sure it protects completely against any race
condition there.
Thierry
You *cannot* look at deliver: in isolation, add:/remove: are the other
pieces of the same puzzle.
Yes, I suspected that as well.
You may optimize for either delivery performance or subscription
performance, the important point is that subscriptions considered while
delivering cannot change during delivery.
Optimized for delivery speed, you swap the entire subcriptions instvar when
add:/remove: (but you also need monitor use in add:/remove: to resolve
simultaneous modifications by multiple threads ), and can then deliver: to
the current subscriptions using do:
Optimized for subscription speed, you add/remove to subscriptions inline
(protected by a monitor), and maintain immutable delivery by iterating on a
copy of the subscriptions *
The current implementation is, as you see, optimized for subscription speed.
It's been discussed before to offer both, as they are both valuable to
different use cases, or even swap between them based on some heuristic, but
none have found the time/need.
This is why my original point was to question the need for a change. As
you point out, there is a lot to take care of when touching this code.
Cheers,
Henry
P.S. deliver:to:startingAt: is NOT only called with index 1, look at the
recursive call in the ifCurtailed: block.
Basically, it ensures all subscribers are processed even when you encounter
an exception in on of the subscription handlers.
Well noted; I didn't consider that. Would forking the announcement
delivery change that behavior?
* There's a pathological case when you don't protect the delivery by the
monitor, where the behaviour is strictly speaking different for an initially
empty, and a non-empty IdentitySet if an adding thread was interrupted in
atNewIndex:put:, due to tally = 0 check in Set do:, but from deliverys
perspective, a subscription is always there or not.
Thanks,
Thierry
