Re: ACL Performance (caching on object basis) (ITS#1523)

[Pierangelo Masarati <masarati@aero.polimi.it>]

> On Monday, 28. January 2002 01:41, Howard Chu wrote:
> > In reviewing these patches, it looks like acl_check_modlist ought to be
> > checking for any value-dependent ACLs as well, but currently isn't. Yes?
> 
> Well, I wasn't sure about side effects so I left it alone. The NULL parameter 
> means that evaluation of ACL does begins from the start, not from a value 
> dependant ACL, so the only effect is a reduced write performance.
> 
> However, looking into the code, it shouldn't be difficult to introduce this 
> here, too, of the patch should be accepted.
> 
> > Also, since the caching is only performed on a per-entry basis, the entire
> > ACLCache structure looks unnecessary. 
> 
> Do you mean the whole idea is unnecessary or do you think the data structure 
> is unnecessary. 

I think ACL checking is desirable as much as it can reduce ACL evaluation 
overhead.  One fist step could be to identify the cases in which per-value
check is not necessary and skip it for multi-valued attributes; I note
you followed this way by defining the "cacheable" flag.  In this case
there's no need for a cache structure.

> 
> I think the idea to evaluate one ACL only once for an entry does make sense. 
> In normal LDAP setups you will only have a handful of ACLs matching for one 
> entry, however in the current setup the ACLs are evaluated for each attribute 
> and each value in the entry. With the pointer to the first value dependant 
> attribute you can reduce this to one ACL evaluation per attribute (in the 
> case of value independant ACLs), but that will generally still mean that you 
> evaluate the same ACL several times. Imagine an entry with 20 attributes, 30 
> values and five matching ACLs. Without any patch, the slapd will have to 
> evaluate 50 ACLs with the pinter to the first value independant ACL 20 and 
> with the complete patch only 5.
> 
> I stored the information about evaluated ACLs in the ACLCache structure since 
> it seemed to me the way to do this, but if you have a better idea about 
> storing this information, feel free to tell it.
> 
> Maybe calling that ACL cache isn't really proper, but I didn't find a better 

I'm sure it's not a matter of naming :)

> name. I also thought about implementing a real, longer lived ACL cache, but 
> that needed to store information about the validity context of the cached 
> ACLs and storing and evaluating this data seemed to me as expensive as 
> evaluation the ACLs themselves.

This is a very interesting point; I agree the farther the thing goes 
the more expensive it is going to be.

> 
> > It also seems to me that nothing is
> > gained from making the ACLCacheEntry a doubly linked list, a single link
> > would be enough since the list is only traversed in one direction.
> 
> You are right about this. If the whole idea seems acceptable, I will rework 
> this.

In my opinion you should rework it.  I don't like to sound pedantic 
but since ACL is __VERY__ critical (there are security and information 
disclosure issues) you should also add as many consistency checks
as possible (memory failure handling, that is basically check malloc 
return values, possibly, in case of failure, reverting to no cache? 
and, since the code is HEAD, assertions on the arguments to speed 
up development and (potential) flaw detection.

At last, the idea is good, but the topic is delicate and needs care.
Your basis looks good, but we'll have to discuss a while before it
gets integrated.

Pierangelo.