Re: [PATCH] AND and OR ACLs

On 05/01/2013 07:28 PM, Amos Jeffries wrote:
> 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.

>> 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.

Great, this matches what I am working on.

>>>> 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.

Sorry, you lost me here. Both the old and the current code restart from
the head (i.e., the beginning) of the current acl_access rule:

>>> - const ACLList *node = head;

vs.

>>> + for (const ACLList *node = head;

and do node matches for every node in that rule (if all acls match):

>>> - while (node) {
>>> - bool nodeMatched = node->matches(this);
...
>>> - node = node->next;

vs.

>>> + for (const ACLList *node = head; node; node = node->next) {
>>> + ... resultBeforeAsync = matchNode(*node, fast);

I do not know why you saw something in the logs that you do see now, but
there is no difference as far as this from-head node rematching aspect
is concerned (AFAIK).

Cheers,

Alex.
Received on Thu May 02 2013 - 03:57:20 MDT