Hi,
I found that the handling of external objects is suboptimal in the image, and I came up with some ideas to make it better. I have a working solution in my image, but it can be even better.
It seems to me that only Semaphores are stored in the external objects array (slot 39 in the special objects array), but in practice any object can be stored there. Does it make sense to support objects of any other kind? If not, then the code could be simplified.
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array. This could be really handy for finding the object in the array, but the VM is not willing to signal these semaphores in the external objects array. I thought that this is because the VM doesn't know anything about this class. So I tried to change Semaphore itself, but after the change the VM refused to signal any semaphores in the external objects array. Changing the class back to the original didn't help either. Without looking at the VM code, I assumed that the VM will signal anything which has the class in the 19th slot of the special objects array, but that doesn't seem to be the case. Is there any way to make this (add an instatnce variable to Semaphore and still get signals from the VM, or use instances of a subclass of Semaphore there) work?
Levente
On 12.10.2014, at 08:04, Levente Uzonyi leves@elte.hu wrote:
Hi,
I found that the handling of external objects is suboptimal in the image, and I came up with some ideas to make it better. I have a working solution in my image, but it can be even better.
It seems to me that only Semaphores are stored in the external objects array (slot 39 in the special objects array), but in practice any object can be stored there. Does it make sense to support objects of any other kind? If not, then the code could be simplified.
Currently the VM uses the ExternalObjectsArray only for semaphores. Other objects use different mechanisms (e.g. SurfacePlugin for external forms). I'm not sure if there is a good use case for storing non-semaphores in ExternalObjectsArray, maybe ask on the vm-dev list?
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array. This could be really handy for finding the object in the array, but the VM is not willing to signal these semaphores in the external objects array. I thought that this is because the VM doesn't know anything about this class. So I tried to change Semaphore itself, but after the change the VM refused to signal any semaphores in the external objects array. Changing the class back to the original didn't help either. Without looking at the VM code, I assumed that the VM will signal anything which has the class in the 19th slot of the special objects array, but that doesn't seem to be the case. Is there any way to make this (add an instatnce variable to Semaphore and still get signals from the VM, or use instances of a subclass of Semaphore there) work?
The VM only checks for the exact class in slot 19, not for subclasses. Adding an inst var to Semaphore itself should be fine, but you cannot use a subclass without changing the VM. Are you sure you put the modified Semaphore class into SpecialObjectsArray after adding the inst var?
- Bert -
Thanks Bert. It turned out that my image was already broken when I tried to add an instance variable to Semaphore. I've finished the implementation and uploaded it to The Inbox. The following snippet will load it into an updated Trunk image:
Installer mc http: 'source.squeak.org'; project: 'inbox'; package: 'Kernel-ul.881'; package: 'System-ul.686'; package: 'Kernel-ul.882'; package: 'Sound-ul.39'; package: 'Network-ul.153'; package: 'Files-ul.139'; install
I decided to go with the complex solution, which gives the best performance, and a simple API for Semaphores, but also has pretty good performance for any other object. And it's backwards compatible in almost all cases.
Levente
On Sun, 12 Oct 2014, Bert Freudenberg wrote:
On 12.10.2014, at 08:04, Levente Uzonyi leves@elte.hu wrote:
Hi,
I found that the handling of external objects is suboptimal in the image, and I came up with some ideas to make it better. I have a working solution in my image, but it can be even better.
It seems to me that only Semaphores are stored in the external objects array (slot 39 in the special objects array), but in practice any object can be stored there. Does it make sense to support objects of any other kind? If not, then the code could be simplified.
Currently the VM uses the ExternalObjectsArray only for semaphores. Other objects use different mechanisms (e.g. SurfacePlugin for external forms). I'm not sure if there is a good use case for storing non-semaphores in ExternalObjectsArray, maybe ask on the vm-dev list?
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array. This could be really handy for finding the object in the array, but the VM is not willing to signal these semaphores in the external objects array. I thought that this is because the VM doesn't know anything about this class. So I tried to change Semaphore itself, but after the change the VM refused to signal any semaphores in the external objects array. Changing the class back to the original didn't help either. Without looking at the VM code, I assumed that the VM will signal anything which has the class in the 19th slot of the special objects array, but that doesn't seem to be the case. Is there any way to make this (add an instatnce variable to Semaphore and still get signals from the VM, or use instances of a subclass of Semaphore there) work?
The VM only checks for the exact class in slot 19, not for subclasses. Adding an inst var to Semaphore itself should be fine, but you cannot use a subclass without changing the VM. Are you sure you put the modified Semaphore class into SpecialObjectsArray after adding the inst var?
- Bert -
Your implementation looks good.
But if we're changing the API, can we get around exposing ExternalSemaphoreTable to user code? It is an implementation detail, after all.
E.g. for file opening:
semaphore := Semaphore new. fileHandle := self primOpen: name asVmPathName forWrite: writeable semaIndex: semaphore registerExternalIndex. fileHandle ifNil: [ semaphore unregisterExternalIndex. semaphore := nil. ^ nil].
I'm not sure about the exact selectors, but this would make the programmer not having to deal with the implementation detail of the table at all.
- Bert -
On 24.10.2014, at 08:22, Levente Uzonyi leves@elte.hu wrote:
Thanks Bert. It turned out that my image was already broken when I tried to add an instance variable to Semaphore. I've finished the implementation and uploaded it to The Inbox. The following snippet will load it into an updated Trunk image:
Installer mc http: 'source.squeak.org'; project: 'inbox'; package: 'Kernel-ul.881'; package: 'System-ul.686'; package: 'Kernel-ul.882'; package: 'Sound-ul.39'; package: 'Network-ul.153'; package: 'Files-ul.139'; install
I decided to go with the complex solution, which gives the best performance, and a simple API for Semaphores, but also has pretty good performance for any other object. And it's backwards compatible in almost all cases.
Levente
On Sun, 12 Oct 2014, Bert Freudenberg wrote:
On 12.10.2014, at 08:04, Levente Uzonyi leves@elte.hu wrote:
Hi,
I found that the handling of external objects is suboptimal in the image, and I came up with some ideas to make it better. I have a working solution in my image, but it can be even better.
It seems to me that only Semaphores are stored in the external objects array (slot 39 in the special objects array), but in practice any object can be stored there. Does it make sense to support objects of any other kind? If not, then the code could be simplified.
Currently the VM uses the ExternalObjectsArray only for semaphores. Other objects use different mechanisms (e.g. SurfacePlugin for external forms). I'm not sure if there is a good use case for storing non-semaphores in ExternalObjectsArray, maybe ask on the vm-dev list?
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array. This could be really handy for finding the object in the array, but the VM is not willing to signal these semaphores in the external objects array. I thought that this is because the VM doesn't know anything about this class. So I tried to change Semaphore itself, but after the change the VM refused to signal any semaphores in the external objects array. Changing the class back to the original didn't help either. Without looking at the VM code, I assumed that the VM will signal anything which has the class in the 19th slot of the special objects array, but that doesn't seem to be the case. Is there any way to make this (add an instatnce variable to Semaphore and still get signals from the VM, or use instances of a subclass of Semaphore there) work?
The VM only checks for the exact class in slot 19, not for subclasses. Adding an inst var to Semaphore itself should be fine, but you cannot use a subclass without changing the VM. Are you sure you put the modified Semaphore class into SpecialObjectsArray after adding the inst var?
- Bert -
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
- Bert -
On 24.10.2014, at 11:44, Bert Freudenberg bert@freudenbergs.de wrote:
Your implementation looks good.
But if we're changing the API, can we get around exposing ExternalSemaphoreTable to user code? It is an implementation detail, after all.
E.g. for file opening:
semaphore := Semaphore new. fileHandle := self primOpen: name asVmPathName forWrite: writeable semaIndex: semaphore registerExternalIndex. fileHandle ifNil: [ semaphore unregisterExternalIndex. semaphore := nil. ^ nil].
I'm not sure about the exact selectors, but this would make the programmer not having to deal with the implementation detail of the table at all.
- Bert -
On 24.10.2014, at 08:22, Levente Uzonyi leves@elte.hu wrote:
Thanks Bert. It turned out that my image was already broken when I tried to add an instance variable to Semaphore. I've finished the implementation and uploaded it to The Inbox. The following snippet will load it into an updated Trunk image:
Installer mc http: 'source.squeak.org'; project: 'inbox'; package: 'Kernel-ul.881'; package: 'System-ul.686'; package: 'Kernel-ul.882'; package: 'Sound-ul.39'; package: 'Network-ul.153'; package: 'Files-ul.139'; install
I decided to go with the complex solution, which gives the best performance, and a simple API for Semaphores, but also has pretty good performance for any other object. And it's backwards compatible in almost all cases.
Levente
On Sun, 12 Oct 2014, Bert Freudenberg wrote:
On 12.10.2014, at 08:04, Levente Uzonyi leves@elte.hu wrote:
Hi,
I found that the handling of external objects is suboptimal in the image, and I came up with some ideas to make it better. I have a working solution in my image, but it can be even better.
It seems to me that only Semaphores are stored in the external objects array (slot 39 in the special objects array), but in practice any object can be stored there. Does it make sense to support objects of any other kind? If not, then the code could be simplified.
Currently the VM uses the ExternalObjectsArray only for semaphores. Other objects use different mechanisms (e.g. SurfacePlugin for external forms). I'm not sure if there is a good use case for storing non-semaphores in ExternalObjectsArray, maybe ask on the vm-dev list?
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array. This could be really handy for finding the object in the array, but the VM is not willing to signal these semaphores in the external objects array. I thought that this is because the VM doesn't know anything about this class. So I tried to change Semaphore itself, but after the change the VM refused to signal any semaphores in the external objects array. Changing the class back to the original didn't help either. Without looking at the VM code, I assumed that the VM will signal anything which has the class in the 19th slot of the special objects array, but that doesn't seem to be the case. Is there any way to make this (add an instatnce variable to Semaphore and still get signals from the VM, or use instances of a subclass of Semaphore there) work?
The VM only checks for the exact class in slot 19, not for subclasses. Adding an inst var to Semaphore itself should be fine, but you cannot use a subclass without changing the VM. Are you sure you put the modified Semaphore class into SpecialObjectsArray after adding the inst var?
- Bert -
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
- Bert -
On 24.10.2014, at 11:44, Bert Freudenberg bert@freudenbergs.de wrote:
Your implementation looks good.
But if we're changing the API, can we get around exposing ExternalSemaphoreTable to user code? It is an implementation detail, after all.
E.g. for file opening:
semaphore := Semaphore new. fileHandle := self primOpen: name asVmPathName forWrite: writeable semaIndex: semaphore registerExternalIndex. fileHandle ifNil: [ semaphore unregisterExternalIndex. semaphore := nil. ^ nil].
I'm not sure about the exact selectors, but this would make the programmer not having to deal with the implementation detail of the table at all.
Yes, it's possible to extend the API by adding a few methods to Semaphore.
Levente
- Bert -
On 24.10.2014, at 08:22, Levente Uzonyi leves@elte.hu wrote:
Thanks Bert. It turned out that my image was already broken when I tried to add an instance variable to Semaphore. I've finished the implementation and uploaded it to The Inbox. The following snippet will load it into an updated Trunk image:
Installer mc http: 'source.squeak.org'; project: 'inbox'; package: 'Kernel-ul.881'; package: 'System-ul.686'; package: 'Kernel-ul.882'; package: 'Sound-ul.39'; package: 'Network-ul.153'; package: 'Files-ul.139'; install
I decided to go with the complex solution, which gives the best performance, and a simple API for Semaphores, but also has pretty good performance for any other object. And it's backwards compatible in almost all cases.
Levente
On Sun, 12 Oct 2014, Bert Freudenberg wrote:
On 12.10.2014, at 08:04, Levente Uzonyi leves@elte.hu wrote:
Hi,
I found that the handling of external objects is suboptimal in the image, and I came up with some ideas to make it better. I have a working solution in my image, but it can be even better.
It seems to me that only Semaphores are stored in the external objects array (slot 39 in the special objects array), but in practice any object can be stored there. Does it make sense to support objects of any other kind? If not, then the code could be simplified.
Currently the VM uses the ExternalObjectsArray only for semaphores. Other objects use different mechanisms (e.g. SurfacePlugin for external forms). I'm not sure if there is a good use case for storing non-semaphores in ExternalObjectsArray, maybe ask on the vm-dev list?
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array. This could be really handy for finding the object in the array, but the VM is not willing to signal these semaphores in the external objects array. I thought that this is because the VM doesn't know anything about this class. So I tried to change Semaphore itself, but after the change the VM refused to signal any semaphores in the external objects array. Changing the class back to the original didn't help either. Without looking at the VM code, I assumed that the VM will signal anything which has the class in the 19th slot of the special objects array, but that doesn't seem to be the case. Is there any way to make this (add an instatnce variable to Semaphore and still get signals from the VM, or use instances of a subclass of Semaphore there) work?
The VM only checks for the exact class in slot 19, not for subclasses. Adding an inst var to Semaphore itself should be fine, but you cannot use a subclass without changing the VM. Are you sure you put the modified Semaphore class into SpecialObjectsArray after adding the inst var?
- Bert -
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I also like the implementation overall, but it seems to me that a Semaphore should not be responsible for knowing about whether somebody used it in the external objects array. It seems more natural for ExternalSemaphoreTable to be responsible for keeping track of the relationship between semaphores and external resources. Thus I don't really like the #indexInExternalObjectsArray instance variable in Semaphore.
On the other hand, in your earlier post introducing this topic, you said:
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array.
So IMO, maybe maybe your original concept is better, even if it requires another class. An ExternalSemaphore would be a Semaphore that knows about its place in the ExternalObjectsArray, but semaphores in general do not need to know about that.
I'm not sure which would be better, but it seems to me that either ExternalSemaphoreTable should be responsible for maintaining the relationship (presumably with an IdentityDictionary), or ExternalSemaphore should be a kind of semaphore that knows something more about its place in the world.
Dave
On Fri, 24 Oct 2014, David T. Lewis wrote:
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I also like the implementation overall, but it seems to me that a Semaphore should not be responsible for knowing about whether somebody used it in the external objects array. It seems more natural for ExternalSemaphoreTable to be responsible for keeping track of the relationship between semaphores and external resources. Thus I don't really like the #indexInExternalObjectsArray instance variable in Semaphore.
On the other hand, in your earlier post introducing this topic, you said:
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array.
So IMO, maybe maybe your original concept is better, even if it requires another class. An ExternalSemaphore would be a Semaphore that knows about its place in the ExternalObjectsArray, but semaphores in general do not need to know about that.
I'm not sure which would be better, but it seems to me that either ExternalSemaphoreTable should be responsible for maintaining the relationship (presumably with an IdentityDictionary), or ExternalSemaphore should be a kind of semaphore that knows something more about its place in the world.
The problem with ExternalSemaphore idea is that it requires VM changes. The VM is not willing to signal instances of non-Semaphores. AFAIK signaling is based on what's in slot 19 of the specialObjectsArray. But changing the class there is not possible, because other VM mechanisms also rely on the content of that slot.
Levente
Dave
On Sat, Oct 25, 2014 at 01:07:31AM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, David T. Lewis wrote:
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I also like the implementation overall, but it seems to me that a Semaphore should not be responsible for knowing about whether somebody used it in the external objects array. It seems more natural for ExternalSemaphoreTable to be responsible for keeping track of the relationship between semaphores and external resources. Thus I don't really like the #indexInExternalObjectsArray instance variable in Semaphore.
On the other hand, in your earlier post introducing this topic, you said:
I also thought that I'll create a special semaphore (ExternalSemaphore - which is a subclass of Semaphore) that knows its own index in the external objects array.
So IMO, maybe maybe your original concept is better, even if it requires another class. An ExternalSemaphore would be a Semaphore that knows about its place in the ExternalObjectsArray, but semaphores in general do not need to know about that.
I'm not sure which would be better, but it seems to me that either ExternalSemaphoreTable should be responsible for maintaining the relationship (presumably with an IdentityDictionary), or ExternalSemaphore should be a kind of semaphore that knows something more about its place in the world.
The problem with ExternalSemaphore idea is that it requires VM changes. The VM is not willing to signal instances of non-Semaphores. AFAIK signaling is based on what's in slot 19 of the specialObjectsArray. But changing the class there is not possible, because other VM mechanisms also rely on the content of that slot.
Hmm... Unfortunately you are right.
InterpreterPrimitives>>primitiveSignal "synchromously signal the semaphore. This may change the active process as a result" | sema | sema := self stackTop. "rcvr" self assertClassOf: sema is: (objectMemory splObj: ClassSemaphore). self successful ifTrue: [ self synchronousSignal: sema ].
Dave
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I don't see the indexesByObjects dictionary actually being updated, except in the case of setting a new externalObjectsArray in the ExternalSemaphoreTable. I think I am overlooking something?
Dave
On Sat, 25 Oct 2014, David T. Lewis wrote:
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I don't see the indexesByObjects dictionary actually being updated, except in the case of setting a new externalObjectsArray in the ExternalSemaphoreTable. I think I am overlooking something?
It's updated in #safelyRegisterExternalObject: and #safelyUnregisterExternalObject:, but - as the class comment states - it's only used for non-Semaphores.
Based on the feedback there's too much resistance to add an instance variable to Semaphore, so I'm about to remove that part from the implementation.
Levente
Dave
On Sat, Oct 25, 2014 at 10:02:22PM +0200, Levente Uzonyi wrote:
On Sat, 25 Oct 2014, David T. Lewis wrote:
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I don't see the indexesByObjects dictionary actually being updated, except in the case of setting a new externalObjectsArray in the ExternalSemaphoreTable. I think I am overlooking something?
It's updated in #safelyRegisterExternalObject: and #safelyUnregisterExternalObject:, but - as the class comment states - it's only used for non-Semaphores.
Based on the feedback there's too much resistance to add an instance variable to Semaphore, so I'm about to remove that part from the implementation.
I don't think the instance variable is horrible, but I do think it would feel cleaner if you can do without it, so +1 if you are able to do that.
Thanks Dave
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
Levente
On Sat, 25 Oct 2014, David T. Lewis wrote:
On Sat, Oct 25, 2014 at 10:02:22PM +0200, Levente Uzonyi wrote:
On Sat, 25 Oct 2014, David T. Lewis wrote:
On Fri, Oct 24, 2014 at 09:52:11PM +0200, Levente Uzonyi wrote:
On Fri, 24 Oct 2014, Bert Freudenberg wrote:
Another thought: is it really necessary to cache the index in the semaphore? Wouldn't scanning the externalObjectsTable for self be efficient enough? It's pretty small, after all.
That's exactly what the current implementation does (scans the table), and this is what I want to avoid, because it doesn't scale. The table is small when you start your image, but it can easily grow large - especially on a server (3 semaphores per socket) - and it never gets shrinked. So once it grows, it'll stay slow "forever".
And no, it's not absolutely necessary to store the index in the semaphores. The new implementation uses an IdentityDictionary to store the indexes of non-semaphores. The same method could work for semaphores too. But it's convenient to store the index in the semaphore, otherwise the method which creates and registers a new semaphore has to return two values - the index and the semaphore - which is cumbersome. It's also faster to ask for a new Semaphore from the ExternalSemaphoreTable, because that way it knows that the Semaphore is not registered.
I don't see the indexesByObjects dictionary actually being updated, except in the case of setting a new externalObjectsArray in the ExternalSemaphoreTable. I think I am overlooking something?
It's updated in #safelyRegisterExternalObject: and #safelyUnregisterExternalObject:, but - as the class comment states - it's only used for non-Semaphores.
Based on the feedback there's too much resistance to add an instance variable to Semaphore, so I'm about to remove that part from the implementation.
I don't think the instance variable is horrible, but I do think it would feel cleaner if you can do without it, so +1 if you are able to do that.
Thanks Dave
On Sun, Oct 26, 2014 at 02:06:20AM +0200, Levente Uzonyi wrote:
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
Levente
I like this a lot. Not only does it provide the ExternalSemaphoreTable improvements, but I also find that when I restart my image with OSProcess loaded, the indexesByObjects dictionary clearly shows the semaphore that was registered for the child process watcher at image startup. Thus an explorer on the ExternalSemaphoreTable will display semaphores associated with processes that wait on them, which also identifies the process waiting on that semaphore. You can see this right in the object explorer.
Very nice!
I note that a Semaphore can still know its external objects index by querying the ExternalSemaphoreTable:
Semaphore>>externalIndex ^ExternalSemaphoreTable externalIndexFor: self
ExternalSemaphoreTable class>>externalIndexFor: anObject ^self current indexesByObjects at: anObject ifAbsent: [nil]
ExternalSemaphoreTable>>indexesByObjects ^indexesByObjects
Dave
On Sat, 25 Oct 2014, David T. Lewis wrote:
On Sun, Oct 26, 2014 at 02:06:20AM +0200, Levente Uzonyi wrote:
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
Levente
I like this a lot. Not only does it provide the ExternalSemaphoreTable improvements, but I also find that when I restart my image with OSProcess loaded, the indexesByObjects dictionary clearly shows the semaphore that was registered for the child process watcher at image startup. Thus an explorer on the ExternalSemaphoreTable will display semaphores associated with processes that wait on them, which also identifies the process waiting on that semaphore. You can see this right in the object explorer.
Very nice!
I note that a Semaphore can still know its external objects index by querying the ExternalSemaphoreTable:
Semaphore>>externalIndex ^ExternalSemaphoreTable externalIndexFor: self
ExternalSemaphoreTable class>>externalIndexFor: anObject ^self current indexesByObjects at: anObject ifAbsent: [nil]
ExternalSemaphoreTable>>indexesByObjects ^indexesByObjects
It's doable, but I don't think it's useful. The index is only meaningful for the plugins. Also, most of the API could be added to Semaphore too as Bert suggested. Btw, accessing #indexesByObjects is no thread-safe this way.
Levente
Dave
On Sun, Oct 26, 2014 at 02:06:20AM +0200, Levente Uzonyi wrote:
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
This looks really good to me. I vote for moving it to trunk.
Dave
On 26.10.2014, at 10:32, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Oct 26, 2014 at 02:06:20AM +0200, Levente Uzonyi wrote:
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
This looks really good to me. I vote for moving it to trunk.
Dave
Yes, the implementation itself looks good.
It's just a bit unfortunate that the user code gets even less generic than it was before.
- Bert -
On Sun, 26 Oct 2014, Bert Freudenberg wrote:
On 26.10.2014, at 10:32, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Oct 26, 2014 at 02:06:20AM +0200, Levente Uzonyi wrote:
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
This looks really good to me. I vote for moving it to trunk.
Dave
Yes, the implementation itself looks good.
It's just a bit unfortunate that the user code gets even less generic than it was before.
What do you mean by "less generic"? Is it that the code says ExternalSemaphoreTable instead of Smalltalk? The code was extracted from Smalltalk more than 14 years ago, because
"It seemed cleaner to deligate the reponsibility here versus adding more code and another class variable to SystemDictionary"
I could have left all (indirect) users of the ExternalSemaphoreTable unchanged, but I think we really should make SmalltalkImage less monolithic and remove those "proxy" methods from it in the near future. The new methods give additional performance boost (because we know that the Semaphores are new, so we don't have to check if they are already included, and because we don't have to wait for the lock multiple times, when multiple Semaphores are registered or unregistered) especially for Sockets, which was the main reason I spent so much time on this.
Levente
- Bert -
On 26.10.2014, at 14:38, Levente Uzonyi leves@elte.hu wrote:
On Sun, 26 Oct 2014, Bert Freudenberg wrote:
Yes, the implementation itself looks good.
It's just a bit unfortunate that the user code gets even less generic than it was before.
What do you mean by "less generic"? Is it that the code says ExternalSemaphoreTable instead of Smalltalk?
Yes.
The code was extracted from Smalltalk more than 14 years ago, because
"It seemed cleaner to deligate the reponsibility here versus adding more code and another class variable to SystemDictionary"
Delegating is cleaner indeed. You took out the delegation and made user code not use the Smalltalk facade anymore, but instead use the implementation directly.
This delegates to ExternalSemaphoreTable (or anywhere else):
Smalltalk unregisterExternalObject: semaphore
This exposes ExternalSemaphoreTable:
ExternalSemaphoreTable unregisterExternalObject: semaphore
E.g., you already mentioned that "ExternalSemaphoreTable" is misnamed since it can store any object. Why spread the use of it everywhere then? If we rename it to "ExternalObjectsTable" later then we are breaking the API and have to change all senders. Much simpler to change the delegation methods.
I could have left all (indirect) users of the ExternalSemaphoreTable unchanged, but I think we really should make SmalltalkImage less monolithic and remove those "proxy" methods from it in the near future.
I see nothing wrong with Smalltalk being a single facade for system services that have no obviously good home elsewhere.
The new methods give additional performance boost (because we know that the Semaphores are new, so we don't have to check if they are already included, and because we don't have to wait for the lock multiple times, when multiple Semaphores are registered or unregistered) especially for Sockets, which was the main reason I spent so much time on this.
I would be surprised if the indirection itself has any real performance impact, and it's simple to add optimizing methods to the facade, too.
I had suggested putting the API in Semaphore itself (maybe on the class side) but since it's not specific to Semaphores I agree it's better to put elsewhere.
- Bert -
Good points. I've uploaded new versions to The Inbox:
Installer squeakInbox package: 'System-ul.688'; package: 'Sound-ul.41'; package: 'Network-ul.155'; package: 'Files-ul.141'; install.
The new class is called ExternalObjectTable. ExternalSemaphoreTable is gone. All API is in SmalltalkImage in a separate category named external objects.
Levente
On Mon, 27 Oct 2014, Bert Freudenberg wrote:
On 26.10.2014, at 14:38, Levente Uzonyi leves@elte.hu wrote:
On Sun, 26 Oct 2014, Bert Freudenberg wrote:
Yes, the implementation itself looks good.
It's just a bit unfortunate that the user code gets even less generic than it was before.
What do you mean by "less generic"? Is it that the code says ExternalSemaphoreTable instead of Smalltalk?
Yes.
The code was extracted from Smalltalk more than 14 years ago, because
"It seemed cleaner to deligate the reponsibility here versus adding more code and another class variable to SystemDictionary"
Delegating is cleaner indeed. You took out the delegation and made user code not use the Smalltalk facade anymore, but instead use the implementation directly.
This delegates to ExternalSemaphoreTable (or anywhere else):
Smalltalk unregisterExternalObject: semaphore
This exposes ExternalSemaphoreTable:
ExternalSemaphoreTable unregisterExternalObject: semaphore
E.g., you already mentioned that "ExternalSemaphoreTable" is misnamed since it can store any object. Why spread the use of it everywhere then? If we rename it to "ExternalObjectsTable" later then we are breaking the API and have to change all senders. Much simpler to change the delegation methods.
I could have left all (indirect) users of the ExternalSemaphoreTable unchanged, but I think we really should make SmalltalkImage less monolithic and remove those "proxy" methods from it in the near future.
I see nothing wrong with Smalltalk being a single facade for system services that have no obviously good home elsewhere.
The new methods give additional performance boost (because we know that the Semaphores are new, so we don't have to check if they are already included, and because we don't have to wait for the lock multiple times, when multiple Semaphores are registered or unregistered) especially for Sockets, which was the main reason I spent so much time on this.
I would be surprised if the indirection itself has any real performance impact, and it's simple to add optimizing methods to the facade, too.
I had suggested putting the API in Semaphore itself (maybe on the class side) but since it's not specific to Semaphores I agree it's better to put elsewhere.
- Bert -
Ship it :)
- Bert -
On 27.10.2014, at 12:01, Levente Uzonyi leves@elte.hu wrote:
Good points. I've uploaded new versions to The Inbox:
Installer squeakInbox package: 'System-ul.688'; package: 'Sound-ul.41'; package: 'Network-ul.155'; package: 'Files-ul.141'; install.
The new class is called ExternalObjectTable. ExternalSemaphoreTable is gone. All API is in SmalltalkImage in a separate category named external objects.
Levente
On Mon, 27 Oct 2014, Bert Freudenberg wrote:
On 26.10.2014, at 14:38, Levente Uzonyi leves@elte.hu wrote:
On Sun, 26 Oct 2014, Bert Freudenberg wrote:
Yes, the implementation itself looks good.
It's just a bit unfortunate that the user code gets even less generic than it was before.
What do you mean by "less generic"? Is it that the code says ExternalSemaphoreTable instead of Smalltalk?
Yes.
The code was extracted from Smalltalk more than 14 years ago, because
"It seemed cleaner to deligate the reponsibility here versus adding more code and another class variable to SystemDictionary"
Delegating is cleaner indeed. You took out the delegation and made user code not use the Smalltalk facade anymore, but instead use the implementation directly.
This delegates to ExternalSemaphoreTable (or anywhere else):
Smalltalk unregisterExternalObject: semaphore
This exposes ExternalSemaphoreTable:
ExternalSemaphoreTable unregisterExternalObject: semaphore
E.g., you already mentioned that "ExternalSemaphoreTable" is misnamed since it can store any object. Why spread the use of it everywhere then? If we rename it to "ExternalObjectsTable" later then we are breaking the API and have to change all senders. Much simpler to change the delegation methods.
I could have left all (indirect) users of the ExternalSemaphoreTable unchanged, but I think we really should make SmalltalkImage less monolithic and remove those "proxy" methods from it in the near future.
I see nothing wrong with Smalltalk being a single facade for system services that have no obviously good home elsewhere.
The new methods give additional performance boost (because we know that the Semaphores are new, so we don't have to check if they are already included, and because we don't have to wait for the lock multiple times, when multiple Semaphores are registered or unregistered) especially for Sockets, which was the main reason I spent so much time on this.
I would be surprised if the indirection itself has any real performance impact, and it's simple to add optimizing methods to the facade, too.
I had suggested putting the API in Semaphore itself (maybe on the class side) but since it's not specific to Semaphores I agree it's better to put elsewhere.
- Bert -
On Mon, Oct 27, 2014 at 12:19:33PM -0400, Bert Freudenberg wrote:
On 27.10.2014, at 12:01, Levente Uzonyi leves@elte.hu wrote:
Good points. I've uploaded new versions to The Inbox:
Installer squeakInbox package: 'System-ul.688'; package: 'Sound-ul.41'; package: 'Network-ul.155'; package: 'Files-ul.141'; install.
The new class is called ExternalObjectTable. ExternalSemaphoreTable is gone. All API is in SmalltalkImage in a separate category named external objects.
Levente
Ship it :)
- Bert -
Excellent. Thank you Levente!
Dave
On Sun, Oct 26, 2014 at 12:36:13PM -0400, Bert Freudenberg wrote:
On 26.10.2014, at 10:32, David T. Lewis lewis@mail.msen.com wrote:
On Sun, Oct 26, 2014 at 02:06:20AM +0200, Levente Uzonyi wrote:
I've uploaded the new versions of the packages to The Inbox, and moved the old ones to The Treated Inbox. The main difference is that the Semaphore class is not modified, and its instances are handled like any other object. There's also a new #cleanUp: method which will allow freeing up some memory. And the caches are shrinked whenever the external objects are wiped. Since Semaphores don't know their indexes anymore, the new part of the API has also changed. The following snippet will load the new versions to an up-to-date Trunk image:
Installer squeakInbox package: 'System-ul.687'; package: 'Sound-ul.40'; package: 'Kernel-ul.883'; package: 'Network-ul.154'; package: 'Files-ul.140'; install
This looks really good to me. I vote for moving it to trunk.
Dave
Yes, the implementation itself looks good.
It's just a bit unfortunate that the user code gets even less generic than it was before.
Bert,
Are you referring to user code that calls e.g. newExternalSemaphoreDo: and receives a semaphore and its index in the external table? This seems quite reasonable to me, and it reads nicely in the Network, File, and Sound code that sends it. Meanwhile, existing code such as OSProcess continues to work without modification.
Dave
On Sat, Oct 25, 2014 at 10:02:22PM +0200, Levente Uzonyi wrote:
On Sat, 25 Oct 2014, David T. Lewis wrote:
I don't see the indexesByObjects dictionary actually being updated, except in the case of setting a new externalObjectsArray in the ExternalSemaphoreTable. I think I am overlooking something?
It's updated in #safelyRegisterExternalObject: and #safelyUnregisterExternalObject:, but - as the class comment states - it's only used for non-Semaphores.
Sorry, I should have looked more carefully. This works exactly as expected and as documented in the class comment:
ExternalSemaphoreTable registerExternalObject: Object new.
Dave
squeak-dev@lists.squeakfoundation.org