[Openais] Re: just looked at some of the openais code, have some comments

[Steven Dake]

On Wed, 2004-06-30 at 13:25, Chris Friesen wrote:
> Steven Dake wrote:
> > Chris,
> > 
> > Take a look at this patch
> > 
> > I think it implements most of what you mentioend regarding ref counting
> > with the destructor idea.
> 
> Looks pretty good.  I do have a few comments/questions though.
> 
> Would it make sense to use the instance mutex for the refcount as well?  It 
> would save on memory, and fundamentally the refcount is part of the instance. 
> This would then let saHandleCreate() initialize the instance mutex.
> 
> 

I was thinking of making this change previously I was just too lazy to
change all the mutex locks and unlocks :)

> 
> When shutting and closing sockets, we must check that the socket is greater than 
> or equal to zero, not just nonzero.  zero is a valid socket number.
> 
> 

your right thanks for the catch.

> 
> In saHandleInstancePut(), we can't free the instance until we've given up the 
> mutexes on both it and the refcount.  It could be done like this:
> 
> int localref;
> lock()
> refcount--;
> localref = refcount;
> unlock
> if (localref == 0)
> 	do cleanup stuff
> 	free
> 
> We have to copy the refcount while under the lock since other threads may be 
> racing with us.  This way only one will have a localref of zero, and the free 
> will only happen once.
> 
> 

I don't understand why this is necessary.  The "instance" variable can
be freed at any time as long as it is not being referenced.  The way it
works now, there cannot be a race, since the only time free occurs is
when the ref count drops to zero.  The access on this reference count is
protected from races by mutex.

I could see some kind of race is gets/puts were uneven, but then the
user of the get/put would be broken and needs fixing :)

> 
> Also, I think that finalize() should explicitly remove the instance from the 
> database even if there are still refcounts on it.  I think it makes sense to 
> ensure that nobody else can get new references after that point.
> 

good idea i'll make this change.

> To do the database removal we could create saHandleDestroy() (as a counterpart 
> to saHandleCreate()).  It would take the database, the handle, and a pointer to 
> the instance.  The implementation becomes:
> 
> lock database
> if passed-in instance matches handle lookup
> 	memset handle lookup (wipes out handle/instance mapping)
> 	saHandleInstancePut() (since we dropped a reference from the database)
> unlock database
> 
> Doing it that way means that saHandleInstancePut() only needs an instance and 
> mutex/refcount offsets.  No database manipulation required, which means no need 
> to aquire database locks.  And I think it makes more sense conceptually for the 
> removal from the database to drop the refcount, rather than artificially 
> dropping the refcount to force database removal.
> 

agreed

Thanks for your great comments and help

-steve

> 
> 
> Finally, a bit of an optimization.  For activatepoll  we send a message to 
> another process, forcing a context switch, so that it can send a message back to 
> us, so that we wake up from poll(). Is there any reason we need to be 
> stream-oriented?  If we were datagram-oriented (which is no less reliable with 
> unix sockets) and connectionless, we could send ourselves a message directly and 
> avoid the context switch.  It would also mean that if a socket was readable, a 
> whole message would be guaranteed to be there.
> 
> 
> 
> 
> Chris