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. Sven
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.
But, when tracing code which is a heavy user of announcements such as MC loads, it just does not register as a loss compared to the time lost processing those announcements. Do you have time profiles where #deliver: register as a significant contributor? I don't. Thierry (who discovered that RPackageOrganizer>>#rpackageNamed: could register as heavy on a MC load :( when tracing a MC load)
On 01 Jul 2015, at 21:07, Thierry Goubier <thierry.goubier@gmail.com> 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.
But, when tracing code which is a heavy user of announcements such as MC loads, it just does not register as a loss compared to the time lost processing those announcements.
Do you have time profiles where #deliver: register as a significant contributor? I don't.
No, I don't either. I said 'when reading the code'. Still, it seems odd. I use Announcements for logging, then speed of handling is (could be) an issue (as is multi-threaded access).
Thierry
(who discovered that RPackageOrganizer>>#rpackageNamed: could register as heavy on a MC load :( when tracing a MC load)
Le 01/07/2015 21:11, Sven Van Caekenberghe a écrit :
On 01 Jul 2015, at 21:07, Thierry Goubier <thierry.goubier@gmail.com> 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.
But, when tracing code which is a heavy user of announcements such as MC loads, it just does not register as a loss compared to the time lost processing those announcements.
Do you have time profiles where #deliver: register as a significant contributor? I don't.
No, I don't either. I said 'when reading the code'. Still, it seems odd.
A few other things are odd in this code.
I use Announcements for logging, then speed of handling is (could be) an issue (as is multi-threaded access).
Agreed. What I would do with that code is: * get rid of #deliver:to:startingAt: because it is only called starting at 1. * inline #subscriptionsHandling: into #deliver: * inline #deliver:to: as well into #deliver: (as a combined loop) * Check that I get the expected speed benefit (I'm probably down at half as many lines as the previous structure, so I win on clarity/compact) It seems that the code may have been structured that way to be able to memoize (ok, to cache) the list of interested subscribers, and it turned out it had no sufficient impact on performance to justify managing the associated caching logic. Thierry
Thierry
(who discovered that RPackageOrganizer>>#rpackageNamed: could register as heavy on a MC load :( when tracing a MC load)
On 01 Jul 2015, at 21:23, Thierry Goubier <thierry.goubier@gmail.com> wrote:
Le 01/07/2015 21:11, Sven Van Caekenberghe a écrit :
On 01 Jul 2015, at 21:07, Thierry Goubier <thierry.goubier@gmail.com> 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.
But, when tracing code which is a heavy user of announcements such as MC loads, it just does not register as a loss compared to the time lost processing those announcements.
Do you have time profiles where #deliver: register as a significant contributor? I don't.
No, I don't either. I said 'when reading the code'. Still, it seems odd.
A few other things are odd in this code.
I use Announcements for logging, then speed of handling is (could be) an issue (as is multi-threaded access).
Agreed.
What I would do with that code is: * get rid of #deliver:to:startingAt: because it is only called starting at 1. * inline #subscriptionsHandling: into #deliver: * inline #deliver:to: as well into #deliver: (as a combined loop) * Check that I get the expected speed benefit (I'm probably down at half as many lines as the previous structure, so I win on clarity/compact)
Totally agree.
It seems that the code may have been structured that way to be able to memoize (ok, to cache) the list of interested subscribers, and it turned out it had no sufficient impact on performance to justify managing the associated caching logic.
Maybe, there is also the comment "using a copy, so subscribers can unsubscribe from announcer " but the price to pay for this seems to high to me.
Thierry
Thierry
(who discovered that RPackageOrganizer>>#rpackageNamed: could register as heavy on a MC load :( when tracing a MC load)
Le 01/07/2015 21:29, Sven Van Caekenberghe a écrit :
On 01 Jul 2015, at 21:23, Thierry Goubier <thierry.goubier@gmail.com> wrote:
Le 01/07/2015 21:11, Sven Van Caekenberghe a écrit :
On 01 Jul 2015, at 21:07, Thierry Goubier <thierry.goubier@gmail.com> 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.
But, when tracing code which is a heavy user of announcements such as MC loads, it just does not register as a loss compared to the time lost processing those announcements.
Do you have time profiles where #deliver: register as a significant contributor? I don't.
No, I don't either. I said 'when reading the code'. Still, it seems odd.
A few other things are odd in this code.
I use Announcements for logging, then speed of handling is (could be) an issue (as is multi-threaded access).
Agreed.
What I would do with that code is: * get rid of #deliver:to:startingAt: because it is only called starting at 1. * inline #subscriptionsHandling: into #deliver: * inline #deliver:to: as well into #deliver: (as a combined loop) * Check that I get the expected speed benefit (I'm probably down at half as many lines as the previous structure, so I win on clarity/compact)
Totally agree.
It seems that the code may have been structured that way to be able to memoize (ok, to cache) the list of interested subscribers, and it turned out it had no sufficient impact on performance to justify managing the associated caching logic.
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
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. 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. 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. * 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. -- View this message in context: http://forum.world.st/SubscriptionRegistry-deliver-tp4835238p4835329.html Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
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
On 02 Jul 2015, at 7:03 , Thierry Goubier <thierry.goubier@gmail.com> 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/0574... <http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2012-January/0574...> 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
Hi Henrik, Le 02/07/2015 10:49, Henrik Johansen a écrit :
The whole thread at http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2012-January/0574... 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.
Very interesting thread, thanks.
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.
Ok, so the behavior on halt (or an error opened on debugger) is to suspend all other notifications on that announcer? Including on the main system announcer? I do have some issues in one of the system announcement where strangely the announcement is received before the real operation is undertaken, and this makes handling it properly an interesting problem. Thierry
On 02 Jul 2015, at 9:40 , Thierry Goubier <thierry.goubier@gmail.com> wrote:
Hi Henrik,
Le 02/07/2015 10:49, Henrik Johansen a écrit :
The whole thread at http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2012-January/0574... 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.
Very interesting thread, thanks.
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.
Ok, so the behavior on halt (or an error opened on debugger) is to suspend all other notifications on that announcer? Including on the main system announcer?
The behaviour is the same as when suspending any thread. If said thread is the only source of announcements, then all notifications are effectively suspended. There is nothing blocking other threads from delivering new announcements though. Unlike UnhandledErrors, a debugger raised responding to a halt will not have forked off in a separate thread, so you can navigate the entire stack, inspecting announcement source / other subscribers, etc. The ifCurtailed: merely ensures that if said process is terminated during delivery (Say, a debugger is closed instead of proceeded), the remaining subscribers will be processed before termination actually occurs.
I do have some issues in one of the system announcement where strangely the announcement is received before the real operation is undertaken, and this makes handling it properly an interesting problem.
That sounds like a pathological erroneous use of announcements we tried to make as hard as possible to actually get to work... There are two facets to this; 1. The announcer relies on a subscriber to do work for it. This is just *wrong*, in that it needlessly complicates things. As per above, this is the case if the system announcer announces that a method has been added, but the actual compilation / class installation happens in a subscriber. Instead, announce that the stuff has been done, at the same place it is being done, after it's done. (if that makes sense :) ) 2. You are subscribing to announcements at the wrong abstraction level. If, for instance, you want to refresh browsers based on updated package contents after a method has been added, you do not subscribe to a base system announcement that tells you code has been compiled and installed in a class. You subscribe to the PackageManager that announces a method has been added to a package. Cheers, Henry *One of the reasons an IdentitySet is used for subscriptions, instead of an OrderedCollection, was to make it harder to write code that relies on announcement delivery order without running into cases like the one you describe
2015-07-03 11:30 GMT+02:00 Henrik Johansen <henrik.s.johansen@veloxit.no>:
On 02 Jul 2015, at 9:40 , Thierry Goubier <thierry.goubier@gmail.com> wrote:
Hi Henrik,
Le 02/07/2015 10:49, Henrik Johansen a écrit :
The whole thread at
http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/2012-January/0574... 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.
Very interesting thread, thanks.
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.
Ok, so the behavior on halt (or an error opened on debugger) is to suspend all other notifications on that announcer? Including on the main system announcer?
The behaviour is the same as when suspending any thread. If said thread is the only source of announcements, then all notifications are effectively suspended. There is nothing blocking other threads from delivering new announcements though. Unlike UnhandledErrors, a debugger raised responding to a halt will not have forked off in a separate thread, so you can navigate the entire stack, inspecting announcement source / other subscribers, etc. The ifCurtailed: merely ensures that if said process is terminated during delivery (Say, a debugger is closed instead of proceeded), the remaining subscribers will be processed before termination actually occurs.
Ok. Then this could mean the following is possible: - Debugging an announcement delivery - With, say, system browsers not properly updated since their delivery is among the suspended deliveries. Or: - Debugging an announcement delivery - Have RPackageOrganizer out of sync (because its delivery is next) - And add a class or a method in this out-of-sync RPackageOrganizer.
I do have some issues in one of the system announcement where strangely
the announcement is received before the real operation is undertaken, and this makes handling it properly an interesting problem.
That sounds like a pathological erroneous use of announcements we tried to make as hard as possible to actually get to work... There are two facets to this;
1. The announcer relies on a subscriber to do work for it. This is just *wrong*, in that it needlessly complicates things. As per above, this is the case if the system announcer announces that a method has been added, but the actual compilation / class installation happens in a subscriber. Instead, announce that the stuff has been done, at the same place it is being done, after it's done. (if that makes sense :) )
Yes, it should be done that way in that particular case, but I believe we haven't solved it. It's the ClassModifiedClassDefinition event which has that behavior.
2. You are subscribing to announcements at the wrong abstraction level. If, for instance, you want to refresh browsers based on updated package contents after a method has been added, you do not subscribe to a base system announcement that tells you code has been compiled and installed in a class. You subscribe to the PackageManager that announces a method has been added to a package.
If you work at that abstraction level, then that's fine. But it's not the case. A system browser has to subscribe to the low-level events ;)
Cheers, Henry
*One of the reasons an IdentitySet is used for subscriptions, instead of an OrderedCollection, was to make it harder to write code that relies on announcement delivery order without running into cases like the one you describe
Which leads to the symptom of a random, not repeatable bug triggered by the particular ordering of the instance of the IdentitySet. Or a non-bug, but random behavior of the delivery. Had that in the Keymapping event handling. Hated that. I understand well the concern, but I honestly wonder if triggering random bugs helps. Regards, Thierry
Hi 2015-07-03 12:54 GMT+03:00 Thierry Goubier <thierry.goubier@gmail.com>:
Had that in the Keymapping event handling. Hated that. I understand well the concern, but I honestly wonder if triggering random bugs helps.
Maybe it is better to use OrderedCollection but always announce events in reverse order. In this case event handling order will be always not equal to subscribing order. And no random errors will happen. Always reproducible case
participants (5)
-
Denis Kudriashov -
Henrik Johansen -
Henrik Sperre Johansen
-
Sven Van Caekenberghe -
Thierry Goubier