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
