> On 02 Jul 2015, at 7:03 , Thierry Goubier <[email protected]> wrote: > > 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?
The whole thread at http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2012-January/057478.html <http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2012-January/057478.html> is a good read for insight into the thoughts behind the current implementation. While some of Lukas' complaints that related to artifacts of the live migration has been resolved, many of the decisions made wrt optimization target/robust delivery are still valid. My thoughts are now as they were then wrt. forking delivery, it makes code both harder to debug, and is (imho) not a good tradeoff between performance/clarity and percieved benefit. I'd rather the handler be responsible for the decision of whether it is heavy enough to be forked off in order to not block other delivery, than enforce it as a default. I haven't searched for the related thread, but the fork-on-exception behaviour was eventually argued down to only apply for UnhandledErrors. The ifCurtailed: block is still somewhat useful, it lets you do things like put halts in handlers, and still be certain remaining subscribers are notified if you close the debugger, rather than proceed. Cheers, Henry
