[Pharo-project] About announcements
On 2 January 2012 10:42, Lukas Renggli <renggli@gmail.com> 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.
2. It tests if the registry is empty: Why would that be necessary in an object-oriented implementation?
not necessary. looks like a optimization.
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.
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.
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it). Now it is still have to iterate over inheritance chain in order to determine if given subscription should receive it or not. Of course you can put all subscriptions to Announcement into one group, so you don't need to check it for every subscription: dict at: Announcement put: group. .... so you can do: group do: [:subscription | subscripton deliver: announcement ]. but that imposing that you have a fixed model for announcement relationship, while with #handles: i can simply make it so, that my announcement class are not inherits from its superclass and so, even if you subscribe to Announcement, you won't receive an announcements of my kind, because it simply doesn't walks an inheritance chain in its #handles: method. also, in the end i don't think it matters. Grouping is more optimal (potentially), but you won't see any difference unless you have hundreds of subscriptions, so i think it is not a big deal. Usually there are few subscriptions held by announcer. And so there is simply no need to bother and grouping them. I cannot imagine an announcer holding hundreds or thousands of subscriptions. Announcer allSubInstances collect: [:e | e numberOfSubscriptions ] an OrderedCollection(0 0 1 0)
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
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements? you can use any object as a subscription selector, just make sure it understands #handles: message. i.e. announcer on: myObject do: [ ... ] where myObject can be any object, not just Announcement (sub)class.
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.
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.
So all these 9 steps are not really necessary (or even wanted) in most case. I doubt that any of them makes the code faster. I am glad that the code works for you even if all of this new functionality is untested. And good luck with the race condition :-)
There is a point in here. But, as I said, I thought that the point is to produce the core by having jenkins strip away unwanted material. Of course, the other way would be to start from the core as a seed and have jenkins produce the current image.
Do you really believe that stripping away unwanted material works? No other open source project in the whole world does it like that, everybody builds distributions on top of a smaller core: Linux, GCC, Gnome, FireFox, ... Even in the supermarket, you typically don't get a vegetable hamper if you just want a some potatoes.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- Best regards, Igor Stasenko.
Ok I suggest that we wait for henrik feedback and turn these points in bug entry issue. On Jan 2, 2012, at 2:45 PM, Igor Stasenko wrote:
On 2 January 2012 10:42, Lukas Renggli <renggli@gmail.com> 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.
2. It tests if the registry is empty: Why would that be necessary in an object-oriented implementation?
not necessary. looks like a optimization.
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.
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.
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it).
Now it is still have to iterate over inheritance chain in order to determine if given subscription should receive it or not. Of course you can put all subscriptions to Announcement into one group, so you don't need to check it for every subscription:
dict at: Announcement put: group. .... so you can do:
group do: [:subscription | subscripton deliver: announcement ].
but that imposing that you have a fixed model for announcement relationship, while with #handles: i can simply make it so, that my announcement class are not inherits from its superclass and so, even if you subscribe to Announcement, you won't receive an announcements of my kind, because it simply doesn't walks an inheritance chain in its #handles: method.
also, in the end i don't think it matters. Grouping is more optimal (potentially), but you won't see any difference unless you have hundreds of subscriptions, so i think it is not a big deal. Usually there are few subscriptions held by announcer. And so there is simply no need to bother and grouping them. I cannot imagine an announcer holding hundreds or thousands of subscriptions.
Announcer allSubInstances collect: [:e | e numberOfSubscriptions ] an OrderedCollection(0 0 1 0)
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
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements?
you can use any object as a subscription selector, just make sure it understands #handles: message. i.e.
announcer on: myObject do: [ ... ] where myObject can be any object, not just Announcement (sub)class.
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.
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.
So all these 9 steps are not really necessary (or even wanted) in most case. I doubt that any of them makes the code faster. I am glad that the code works for you even if all of this new functionality is untested. And good luck with the race condition :-)
There is a point in here. But, as I said, I thought that the point is to produce the core by having jenkins strip away unwanted material. Of course, the other way would be to start from the core as a seed and have jenkins produce the current image.
Do you really believe that stripping away unwanted material works? No other open source project in the whole world does it like that, everybody builds distributions on top of a smaller core: Linux, GCC, Gnome, FireFox, ... Even in the supermarket, you typically don't get a vegetable hamper if you just want a some potatoes.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- Best regards, Igor Stasenko.
Ok, first of all I did not know who wrote the code. I was just fixing some of my code that suddenly broke and was astonished what the announcer all does when stepping through the announcement procedure with the debugger ...
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.
A semaphore might block, because an announcement could register another announcement. So probably the Mutex would be the right thing to use.
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.
Array. Slower for registration, but faster at announcing (the common operation).
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it).
Fair enough, probably it doesn't matter.
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements?
If you have a client with 1000 different possibly dynamically changing properties and you want a way to register for announcements of a single property. The current implementation forces you to create a class per property, previously you could fake a "kind of announcements" with an instance.
you can use any object as  a subscription selector, just make sure it understands #handles: message. i.e.
No, because the Announcement>>#handles: takes a class, not an instance.
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.
I didn't complain about #cull:, but about the useless announcer passed in. Now for me you don't need to change the semantics of the Pharo Announcements again. Now for my code I will just use my own implementation that has exactly the behavior and performance characteristics I need. Lukas -- Lukas Renggli www.lukas-renggli.ch
Can one of you turn the following list into bug entries? So that we can make progress by killing them one by one? Stef
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.
A semaphore might block, because an announcement could register another announcement. So probably the Mutex would be the right thing to use.
But apparently Mutex is broken :(
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.
Array. Slower for registration, but faster at announcing (the common operation).
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it).
Fair enough, probably it doesn't matter.
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements?
If you have a client with 1000 different possibly dynamically changing properties and you want a way to register for announcements of a single property. The current implementation forces you to create a class per property, previously you could fake a "kind of announcements" with an instance.
you can use any object as a subscription selector, just make sure it understands #handles: message. i.e.
No, because the Announcement>>#handles: takes a class, not an instance.
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.
I didn't complain about #cull:, but about the useless announcer passed in.
Now for me you don't need to change the semantics of the Pharo Announcements again. Now for my code I will just use my own implementation that has exactly the behavior and performance characteristics I need.
Lukas our state of mind is that we want the infrastructure gets better so we like to change for a better one if this better one is better. If everybody has its own version then we do not improve the infrastructure. Stef
Now for me you don't need to change the semantics of the Pharo Announcements again. Now for my code I will just use my own implementation that has exactly the behavior and performance characteristics I need.
Lukas our state of mind is that we want the infrastructure gets better so we like to change for a better one if this better one is better.
Seems that the understanding of what is better/worse and what is progress/regress is rather different among people.
If everybody has its own version then we do not improve the infrastructure.
Performance issues aside, the current implementation forces me to use a different implementation because Pharo's announcements have a different implementation of selecting the handler (point 7). However, the really weird thing is that Pharo's announcements are now suddenly inconsistent with the Pharo exceptions and how they select the handler. Do you think this is better or worse? Lukas -- Lukas Renggli www.lukas-renggli.ch
On 2 January 2012 21:04, Lukas Renggli <renggli@gmail.com> wrote:
Now for me you don't need to change the semantics of the Pharo Announcements again. Now for my code I will just use my own implementation that has exactly the behavior and performance characteristics I need.
Lukas our state of mind is that we want the infrastructure gets better so we like to change for a better one if this better one is better.
Seems that the understanding of what is better/worse and what is progress/regress is rather different among people.
If everybody has its own version then we do not improve the infrastructure.
Performance issues aside, the current implementation forces me to use a different implementation because Pharo's announcements have a different implementation of selecting the handler (point 7). However, the really weird thing is that Pharo's announcements are now suddenly inconsistent with the Pharo exceptions and how they select the handler. Do you think this is better or worse?
you lost me here. why announcements have to be consistent with exceptions? something which is completely orthogonal to them? it is like saying that apples are inconsistent with oranges. aside of that, i added the changeset, so you can now choose whether your announcement subscription is handling announcement or not based on announcement object itself, not its class. see http://code.google.com/p/pharo/issues/detail?id=5145 -- Best regards, Igor Stasenko.
If everybody has its own version then we do not improve the infrastructure.
Performance issues aside, the current implementation forces me to use a different implementation because Pharo's announcements have a different implementation of selecting the handler (point 7). However, the really weird thing is that Pharo's announcements are now suddenly inconsistent with the Pharo exceptions and how they select the handler. Do you think this is better or worse?
you lost me here. why announcements have to be consistent with exceptions? something which is completely orthogonal to them?
Exceptions and Announcements used to implement a very similar protocol: see their implementation of X class>>#handles:, X>>on:do:, XSet>>#handles:, XSet>>#, etc. Consistency makes frameworks easy to understand and use.
it is like saying that apples are inconsistent with oranges.
- Apples and oranges are inconsistent the way they are eaten (oranges have to be piled first and come in slices). - Apples and pears are consistent the way they are eaten (even if they taste very different). If that is too abstract replace Apples => Announcements Oranges => change:/update: Pears => Exceptions Lukas
aside of that, i added the changeset, so you can now choose whether your announcement subscription is handling announcement or not based on announcement object itself, not its class. see http://code.google.com/p/pharo/issues/detail?id=5145
-- Best regards, Igor Stasenko.
-- Lukas Renggli www.lukas-renggli.ch
On 3 January 2012 14:20, Lukas Renggli <renggli@gmail.com> wrote:
If everybody has its own version then we do not improve the infrastructure.
Performance issues aside, the current implementation forces me to use a different implementation because Pharo's announcements have a different implementation of selecting the handler (point 7). However, the really weird thing is that Pharo's announcements are now suddenly inconsistent with the Pharo exceptions and how they select the handler. Do you think this is better or worse?
you lost me here. why announcements have to be consistent with exceptions? something which is completely orthogonal to them?
Exceptions and Announcements used to implement a very similar protocol: see their implementation of X class>>#handles:, X>>on:do:, XSet>>#handles:, XSet>>#, etc.
as well as it could serve as a source of confusion, because you using same protocol for different purposes in two different frameworks. A medal always has two sides :)
Consistency makes frameworks easy to understand and use.
it is like saying that apples are inconsistent with oranges.
- Apples and oranges are inconsistent the way they are eaten (oranges have to be piled first and come in slices).
- Apples and pears are consistent the way they are eaten (even if they taste very different).
If that is too abstract replace
 Apples => Announcements  Oranges => change:/update:  Pears => Exceptions
your analogy having a small glitch: all those objects sharing common property - they can be eaten. now in smalltalk you can do anything, any object can be 'eaten'. but it doesn't means that they should, otherwise we're just put all behavior in Object class, and use subclasses as a dumb state holders (color, taste, hasToBePiled etc) Exceptions are conceptually different from Announcements in a way they should be used and handled, and i don't see a great value of having synergy between them. If we keep driving analogies, i think a more precise would be to tell that exceptions are to be 'eaten' and announcements are to be 'heard'. And in that way there is no consistency at all. Actually a logic with using inheritance chain to determine whether announcement should be delivered to subscription or not is IMO a major flaw, borrowed from Exceptions. Because testing a class membership is an expensive operation and if you are so concerned about performance, this is a first thing which you would be likely remove.
Lukas
aside of that, i added the changeset, so you can now choose whether your announcement subscription is handling announcement or not based on announcement object itself, not its class. see http://code.google.com/p/pharo/issues/detail?id=5145
-- Best regards, Igor Stasenko.
-- Lukas Renggli www.lukas-renggli.ch
-- Best regards, Igor Stasenko.
Igor Stasenko wrote
as well as it could serve as a source of confusion, because you using same protocol for different purposes in two different frameworks. A medal always has two sides :)
Consistency makes frameworks easy to understand and use.
I think having the two frameworks have the same API would be much easier than if they were almost the same. And I don't think that similarity would cause confusion - quite the opposite. When working with a different framework, I've already shifted my context to the abstractions of that framework. However, my fingers naturally want to type/select-from-autocomplete/etc. the exact same things they've done before, and I don't have to look at a browser to see "how was that selector slightly different from the other one?" -- View this message in context: http://forum.world.st/About-announcements-tp4253255p4257251.html Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
On 3 January 2012 16:24, Sean P. DeNigris <sean@clipperadams.com> wrote:
Igor Stasenko wrote
as well as it could serve as a source of confusion, because you using same protocol for different purposes in two different frameworks. A medal always has two sides :)
Consistency makes frameworks easy to understand and use.
I think having the two frameworks have the same API would be much easier than if they were almost the same. And I don't think that similarity would cause confusion - quite the opposite. When working with a different framework, I've already shifted my context to the abstractions of that framework. However, my fingers naturally want to type/select-from-autocomplete/etc. the exact same things they've done before, and I don't have to look at a browser to see "how was that selector slightly different from the other one?"
exactly. just few days ago, we were hacking new events model with Stef. and i wrongly implemented #handleEvent: which is sent by InputEventFetcher and actually Sensor should just put an event into own queue, because this message were send in fetcher process. while event handling happens inside ui process. with announcements i had to rename #handles: to #handlesAnnouncement: first because if i would not do so, it would be not possible to migrate code (because handles: expect announcement class as argument, and if i change the sender, it will break it) or if i change all implementors of #handles: i will also break it because announcers are still sending this message with wrong argument. well, maybe it is not a big deal. and we could just use #handles: but imo #handlesAnnouncement: looks more explicit (now you have less chances to be confused with the role or receiver - an announcement filter and role of argument - an announced object)
-- View this message in context: http://forum.world.st/About-announcements-tp4253255p4257251.html Sent from the Pharo Smalltalk mailing list archive at Nabble.com.
-- Best regards, Igor Stasenko.
On 2 January 2012 16:12, Lukas Renggli <renggli@gmail.com> wrote:
Ok, first of all I did not know who wrote the code. I was just fixing some of my code that suddenly broke and was astonished what the announcer all does when stepping through the announcement procedure with the debugger ...
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.
A semaphore might block, because an announcement could register another announcement. So probably the Mutex would be the right thing to use.
no, the trick there is that we leaving a critical section before returning (or calling) client code, so there is no chance that it may recurse (attempt enter critical section twice) due to some weird client code.
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.
Array. Slower for registration, but faster at announcing (the common operation).
maybe.
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it).
Fair enough, probably it doesn't matter.
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements?
If you have a client with 1000 different possibly dynamically changing properties and you want a way to register for announcements of a single property. The current implementation forces you to create a class per property, previously you could fake a "kind of announcements" with an instance.
you can use any object as  a subscription selector, just make sure it understands #handles: message. i.e.
No, because the Announcement>>#handles: takes a class, not an instance.
you mean here: deliver: anAnnouncement " deliver an announcement to receiver. In case of failure, it will be handled in separate process" ^ (self handles: anAnnouncement class ) ifTrue: [ [action cull: anAnnouncement cull: announcer] on: UnhandledError fork: [:ex | ex pass ]] we can simply change the 'anAnnouncement class' to just 'anAnnouncement' so the receiver of #handles: will see an announcement itself not its class. It is probably right thing to do, because then you can do: announcer on: foo do: [... ]. announcer announce: 10. Then foo will receive: foo handles: 10. and in your own #handles: implementation you can do something like: handles: anAnnouncement ^ anAnnouncement isInteger
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.
I didn't complain about #cull:, but about the useless announcer passed in.
Now for me you don't need to change the semantics of the Pharo Announcements again. Now for my code I will just use my own implementation that has exactly the behavior and performance characteristics I need.
-- Best regards, Igor Stasenko.
No, because the Announcement>>#handles: takes a class, not an instance.
you mean here:
deliver: anAnnouncement     " deliver an announcement to receiver. In case of failure, it will be handled in separate process"
    ^ (self handles: anAnnouncement class ) ifTrue: [         [action cull: anAnnouncement cull: announcer]             on: UnhandledError fork: [:ex | ex pass ]]
we can simply change the 'anAnnouncement class' to just 'anAnnouncement' so the receiver of #handles: will see an announcement itself not its class.
It is probably right thing to do, because then you can do:
announcer on: foo do: [... ].
announcer announce: 10.
Then foo will receive:
foo handles: 10. and in your own #handles: implementation you can do something like:
handles: anAnnouncement  ^ anAnnouncement isInteger
Yes, #handles: should take an instance not a class. And #handles: should be implemented as Announcement class>>#handles: anAnnouncement ^ anAnnouncement isKindOf: self I guess this is partially my fault, because I called the argument anAnnouncementClass even if I passed in an instance. Lukas -- Lukas Renggli www.lukas-renggli.ch
Thanks Lukas for the detailed points. These are great because they are to the point and they lead to hands-on solutions. And, thanks Igor for picking it up. If Henrik would join the discussion we would have all parties involved and action can just start. And it was so easy. Cheers, Doru On 2 Jan 2012, at 14:45, Igor Stasenko wrote:
On 2 January 2012 10:42, Lukas Renggli <renggli@gmail.com> 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.
2. It tests if the registry is empty: Why would that be necessary in an object-oriented implementation?
not necessary. looks like a optimization.
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.
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.
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it).
Now it is still have to iterate over inheritance chain in order to determine if given subscription should receive it or not. Of course you can put all subscriptions to Announcement into one group, so you don't need to check it for every subscription:
dict at: Announcement put: group. .... so you can do:
group do: [:subscription | subscripton deliver: announcement ].
but that imposing that you have a fixed model for announcement relationship, while with #handles: i can simply make it so, that my announcement class are not inherits from its superclass and so, even if you subscribe to Announcement, you won't receive an announcements of my kind, because it simply doesn't walks an inheritance chain in its #handles: method.
also, in the end i don't think it matters. Grouping is more optimal (potentially), but you won't see any difference unless you have hundreds of subscriptions, so i think it is not a big deal. Usually there are few subscriptions held by announcer. And so there is simply no need to bother and grouping them. I cannot imagine an announcer holding hundreds or thousands of subscriptions.
Announcer allSubInstances collect: [:e | e numberOfSubscriptions ] an OrderedCollection(0 0 1 0)
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
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements?
you can use any object as a subscription selector, just make sure it understands #handles: message. i.e.
announcer on: myObject do: [ ... ] where myObject can be any object, not just Announcement (sub)class.
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.
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.
So all these 9 steps are not really necessary (or even wanted) in most case. I doubt that any of them makes the code faster. I am glad that the code works for you even if all of this new functionality is untested. And good luck with the race condition :-)
There is a point in here. But, as I said, I thought that the point is to produce the core by having jenkins strip away unwanted material. Of course, the other way would be to start from the core as a seed and have jenkins produce the current image.
Do you really believe that stripping away unwanted material works? No other open source project in the whole world does it like that, everybody builds distributions on top of a smaller core: Linux, GCC, Gnome, FireFox, ... Even in the supermarket, you typically don't get a vegetable hamper if you just want a some potatoes.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- Best regards, Igor Stasenko.
-- www.tudorgirba.com "Some battles are better lost than fought."
Thanks Lukas for the detailed points. These are great because they are to the point and they lead to hands-on solutions.
And, thanks Igor for picking it up. If Henrik would join the discussion we would have all parties involved and action can just start.
And it was so easy.
indeed constructivist feedback (even negative). We were working on event and it also seems that Monitor in announcement was causing some problems. Now I would really like to know if the Monitor implementation is good. Stef
Cheers, Doru
On 2 Jan 2012, at 14:45, Igor Stasenko wrote:
On 2 January 2012 10:42, Lukas Renggli <renggli@gmail.com> 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.
2. It tests if the registry is empty: Why would that be necessary in an object-oriented implementation?
not necessary. looks like a optimization.
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.
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.
5. It iterates over all announcements, it doesn't even try to group announcements of the same kind.
it is pointless to group them, and makes no real difference. Because announcer doesn't knows what kinds of announcements will be announcement, and it knows only about subscriptions. Suppose i have a subscription to Announcement. And i announcing AnnouncementA (a subclass of it).
Now it is still have to iterate over inheritance chain in order to determine if given subscription should receive it or not. Of course you can put all subscriptions to Announcement into one group, so you don't need to check it for every subscription:
dict at: Announcement put: group. .... so you can do:
group do: [:subscription | subscripton deliver: announcement ].
but that imposing that you have a fixed model for announcement relationship, while with #handles: i can simply make it so, that my announcement class are not inherits from its superclass and so, even if you subscribe to Announcement, you won't receive an announcements of my kind, because it simply doesn't walks an inheritance chain in its #handles: method.
also, in the end i don't think it matters. Grouping is more optimal (potentially), but you won't see any difference unless you have hundreds of subscriptions, so i think it is not a big deal. Usually there are few subscriptions held by announcer. And so there is simply no need to bother and grouping them. I cannot imagine an announcer holding hundreds or thousands of subscriptions.
Announcer allSubInstances collect: [:e | e numberOfSubscriptions ] an OrderedCollection(0 0 1 0)
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
7. It then tests each announcement if it matches, again it doesn't even try to group the announcements of the same kind. Aside, the match test was changed so that it doesn't allow instance-based announcements anymore, breaking some of my code.
what is instance-based announcements?
you can use any object as a subscription selector, just make sure it understands #handles: message. i.e.
announcer on: myObject do: [ ... ] where myObject can be any object, not just Announcement (sub)class.
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.
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.
So all these 9 steps are not really necessary (or even wanted) in most case. I doubt that any of them makes the code faster. I am glad that the code works for you even if all of this new functionality is untested. And good luck with the race condition :-)
There is a point in here. But, as I said, I thought that the point is to produce the core by having jenkins strip away unwanted material. Of course, the other way would be to start from the core as a seed and have jenkins produce the current image.
Do you really believe that stripping away unwanted material works? No other open source project in the whole world does it like that, everybody builds distributions on top of a smaller core: Linux, GCC, Gnome, FireFox, ... Even in the supermarket, you typically don't get a vegetable hamper if you just want a some potatoes.
Lukas
-- Lukas Renggli www.lukas-renggli.ch
-- Best regards, Igor Stasenko.
-- www.tudorgirba.com
"Some battles are better lost than fought."
On 02.01.2012 14:45, Igor Stasenko wrote:
On 2 January 2012 10:42, Lukas Renggli<renggli@gmail.com> 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
participants (6)
-
Henrik Sperre Johansen -
Igor Stasenko -
Lukas Renggli -
Sean P. DeNigris -
Stéphane Ducasse -
Tudor Girba