Re: [PATCH] AND and OR ACLs v15

On 05/21/2013 08:59 AM, Alex Rousskov wrote:
> Amos,
>
> I do not think it makes sense for the two of us to argue about this
> further. This is a design decision, with pros and cons on both sides,
> and no way to logically prove the other side wrong. Our opinions differ,
> and each of us feels strong enough about this issue to defend their
> view. We can do another round of arguments (and I certainly have
> ammunition to dispute all of your latest assertions!), but I doubt
> either of us will change his position after another round.
>
> Unfortunately, I only got one private comment about this so far
> (supporting my point of view). If there is still not enough public
> discussion to form a different consensus, I will use your +0 vote to
> commit the latest version (where all equally named ACL lines are ORed,
> like they are today) in a few days.

I committed the changes to trunk as r12859, after addressing "after
review" XXXs and applying a source formatting script. Make check in src/
still works, but I was not able to run "make distcheck" on the very
latest version of the code because squidclient(?) build got broken in
trunk before my commit.

After giving it some additional thought, I left the following code in
src/acl/forward.h:

> // TODO: Consider renaming all users and removing. Cons: hides the difference
> // between ACLList tree without actions and acl_access Tree with actions.
> #define acl_access Acl::Tree
> #define ACLList Acl::Tree

While it would not be very difficult to change all users of old
acl_access and ACLList types, it would introduce widespread changes to
the areas of the code unrelated to this commit. It will also hide the
difference between ACLList tree without actions and acl_access Tree with
actions (the new Acl::Tree class handles both). I decided it would be
prudent to get more experience with the new code before that difference
is erased.

In addition to the usual functionality testing, we put the new ACLs
through some performance tests with Web Polygraph. We were able to
construct artificial tests with a large number of ACLs where the new
code was a lot faster (because it does not recheck previous ACLs upon
returning from an async lookup). However, AFAICT, most configurations
will not see a significant difference in performance due to automatic
ACL parameter rearranging and result caching that make such rechecks
relatively cheap compared to slow ACLs that force the rechecks.

The difference in RAM usage after parsing ACLs was negligible in our tests.

HTH,

Alex.
Received on Tue May 28 2013 - 17:03:26 MDT