Re: [PATCH] AND and OR ACLs

On 2/05/2013 10:52 a.m., Alex Rousskov wrote:
> On 12/21/2012 02:05 AM, Amos Jeffries wrote:
>
>> On 21/12/2012 6:28 a.m., Tsantilas Christos wrote:
>>> - The new acls converted to Checklists in order to computed.
>> I really have a big issue with designing it this way as well. The
>> overheads of setting up many Checklists are way out of proportion
>> compared to the problem avoided...
>> IMO design these ACLs to operate as nodes in the ACL test tree which
>> operate on the one Checklist same as all other ACLs.
> Hi Amos,
>
> I am trying to rewrite this patch to address your concerns. I want
> to make sure the rewritten patch does not face the same "overhead"
> objection. Can you clarify why you think that creating something like a
> Checklist (with multiple ACLs) is a lot more expensive than creating a
> typical ACL (with multiple values)?

The issue I had with the earlier version was that each ACL test involved
allocating a whole new ACLFilledChecklist object and initializing it.

What I had been evisioning when we discussed this earlier was a sleek
little design where the ACL AND/OR type was a pre-constructed tree with
the node match() function doing the logic of whether to walk down
sub-ACL A or sub-ACL B to produce its own result. The whole thing using
1 ACLFilledChecklist across the entire process in the same way that 1
ACLFilledChecklist is used today for the whole set of *_access lines no
matter how long or complex they are.

>
>>> My question is, is this any reason why squid is checking from the
>>> beginning the access line when an acl needs lookup?
>>> Is n't it add a performance overhead?
>> It is an artifact of the ACL processing simplification Alex added a
>> while back.
> I do not think I added any code that rechecks access lines from the
> beginning on async lookups! We were restarting the checks from "head"
> before r12176, if that is the revision you are implicating:
>
>> - const ACLList *node = head;
>> - while (node) {
>> - bool nodeMatched = node->matches(this);
>> + for (const ACLList *node = head; node; node = node->next) {
>> + const NodeMatchingResult resultBeforeAsync = matchNode(*node, fast);

Eureka. That would explain why I have not been able to see a regression.
The previous behaviour we would look at teh logs and see a full ACL test
with no re-scan of a part in the middle, nowdays we can clearly see the
line being restarted from scratch. The above would mean that previously
we were overlooking a partial scan from head to some position Foo before
the scan which was seen as being the "whole" scan.

My comment stands though, the from-line restart is an artifact of your
change. As you say it was from-head. Both situations are bad, with the
current somewhat better than before.

>
> I think these rechecks are expensive, and I have an idea on how we can
> avoid them, but doing so would be outside this project scope.

If this update can fix it easily I think we should take the chance.
Otherwise yes leave it.

The model above with sub-ACL matches re-using the parents
ACLFilledChecklist for its data will sidestep the whole bug anyway.

Amos
Received on Thu May 02 2013 - 01:28:23 MDT