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