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