Evan,
Thanks for your analysis of the problems potential problems in the
database. I agree whole heartedly in some ways but disagree in others.
> Ummm, the changes that you requested have been made, but I have grave
> reservations about slpd_database keeping track of the buffers used by
> ProcessAttrRqst(). If I understand the original implementation correctly, then
>
> a) ProcessAttrRqst() -- Allocates an array of SLPDDatabaseAttr structs, each
> of which has a pointer to the attribute string.
> b) ProcessAttrRqst() calls SLPDDatabaseFindAttr() -- SLPDDatabaseAttr() is
> responsible for populating the array of SLPDDatabaseAttr structs. The
> attribute pointers are pointed at the attribute strings kept in the
> database.
> c) ProcessAttrRqst() -- Sends the attribute responses. Does NOT deallocate
> the attribute strings pointed to by the SLPDDatabaseAttr structs.
This is correct.
> This implementation has a couple of issues.
>
> 1. Between steps (b) and (c) we trust that nothing deallocates the attribute
> strings, but there is no mechanism to ensure this assumption. This seems
> somewhat dangerous. (Granted, since we're currently singly threaded, it's not a
> big deal, but it seems to be asking for trouble). I can see two situations
> where this will cause problems: on service aging, and on re-registration.
You are right. Service aging and re-registration could both invalidate
pointers held by the caller of SLPDDatabaseAttr() or
SLPDDatabaseFindSrv(). However, slpd will never be multi-threaded so
there is no chance that either of these events would interrupt a call
path from the network engine. I can't think of any reason to change to
a multi-threaded design. SLP performance tests have shown the single
threaded slpd to be a remarkably scalable DA. The only bottleneck is
network bandwidth unless thousands of services are being registered. In
this case, slpd performance suffers only because the linked-list
database searches are linear. A multi-threaded design not would solve
this bottle neck. There'd be way too many syncronization issues and the
resulting application would probably be slower than if it were single
threaded. Instead, the solution is to re-write the database to use a
different data structure (hash table) -- which could be done without
changing the database interface.
> 2. At step (b) there is the assumption that the database keeps a statically
> (over the duration of the call chain at least) allocated copy of the attribute
> string. Although this can be made true for the complete attribute string, it is
> patently false if a UA is requesting a subset of the attribute string: the
> database must generate that string on a per-request basis (ie, if the UA only
> wants a few of the attributes, that attribute string must be specially
> allocated and returned).
Yes. This is a problem and is the reason why honoring the taglist in
AttrRqsts has been delayed. My solution to the problem is very similar
to what you describe below.
> How big a problem are these?
>
> 1. Isn't a big deal. It's a fairly major violation of encapsulation, but so
> long as we remain singly threaded it's safe. You make mention of moving to a
> more scalable representation of the database. Such a representation may be a
> lot less forgiving about violating encapsulation.
A more scalable representation would still not be required to be
reentrant so I do not think that moving to new type of database would
cause any problems. I agree that we would probably not be able to use
any existing database code (or code snippets) with out modification.
> 2. This is an issue. If we really want to save copying the attribute string,
> I'd suggest keeping a copy of the serialized attributes in the
> SLPDDatabaseEntry -- I assume the most common attribute request will ask for
> the entire string. Requests for subsets of the attributes could generate a
> seperate string, whose deallocation would be protected by a flag on
> SLPDDatabaseAttr struct (ie, the flag would be true for specially allocated
> strings causing it to be deallocated at (c), but false for the entire string).
After your changes to the slpattr calls I was planning on making the
following changes: The SLPDDatabaseEntry needs keep track of 2
buffers. Like you said, one of these buffers will contain the entire
attribute list, and the other will contain the (shorter) list of
serialized attributes in the event that the taglist is used. We will
allocate the buffer that contains the full attribute list on
registration (or re-registration). The buffer that contains the partial
attribute list will be allocated when the first call using a taglist is
received. Both buffers will be deallocated on de-registration or
age-out.
I am glad that you have put some thought into the database <--> network
engine interface problems. With the exception of #2 I do not think that
there are any grave concerns about having SLPDDatabaseEntry keep track
of memory. You are right about breaking traditional encapsulation
boundaries by only returning pointers to database memory rather than
separately allocated buffers. There is a really good reason for doing
this... Have you ever stepped through a call to malloc() realloc(),
strdup(), free(), new or delete? These calls are not simplistic by any
means. They are often very costly even to down to the kernel level.
IMHO a very easy way to increase the efficiency and speed of an
application is to consciously reduce calls to the allocator and
associated temporary memory copies (I think you'd be surprised at how
much time is allocating and copying memory for temporary buffers) The
most common messages that slpd will receive are SrvRqst and AttrRqst.
To respond to these calls slpd usually does not make a single call to
the allocator. This was an intentional design goal that makes slpd's
overall usage of the allocator very efficient. I have not yet looked at
the slpattr or predicate code to see we can still make this statement,
but there is no reason that we can not set things up on SrvReg such that
the calls to satisfy SrvRqst and AttrRqst can not be optimal.
The break in encapsulation still bothers me from an organizational
standpoint (because it is not very intuitive or clean), but given the
functional benefits, I think it is appropriate.
Comments? If not, I will go ahead and pull in your slpattr/predicate
changes and make the modifications to slpd_database.c that I have
described above.
--
Matthew Peterson
Sr. Software Engineer
Caldera Systems, Inc
mpeterson@...
|