[squid-dev] [PATCH] Rework acl/RegexData optimization to use SBufList

[Alex Rousskov]

On 11/13/2016 05:11 PM, Kinkie wrote:

> the attached patch moves away from hand-rolling a c-string onto
> joining a SBufList for optimizing regexes in RegexData.cc.

> You can find attached as a test case the output of squidclient
> mgr:config taken on trunk and on the submitted code. It is slightly
> different in that the new code allows for a longer optimized regex.

I see the following before/after difference:

  -acl bigacl dstdom_regex ... (pattern84) (pattern86) ...
  +acl bigacl dstdom_regex ... (pattern84)|(pattern85) ...

Does changing space into "|" result in one internal regex instead of
two? And that is the optimization you have implemented? How is that
related to SBufs (i.e., why could not old c-strings accommodate longer
regexes)?

And why is there no pattern85 in the "before" test?!


> The code is expected to cause a small performance regression on
> parse and reconfigure due to extra data copies. The regression is
> negligible: the attached test cases perform "squid -k parse" in 60
> msec in trunk and 62 on the branch on a warm cache on my i5 macbook
> air.

A 3% performance regression is not necessarily negligible. Have you
tested with more ACL lines (not larger individual ACL lines) that take a
few seconds to load (as opposed to 60ms) ? If not, please do unless that
test would be irrelevant somehow.

And what is the expected performance improvement from having fewer
longer regexes?


> [RFC]

This should be a different thread IMHO.


> The code managing case-insensitivity flags is IMO quite complicated
> and not really intuitive: it switches between case-insensitive and
> case-sensitive each time it meets a -i flag. 

It does?! Wow! The documentation says that +i switches back to
case-sensitive rather than repeating -i values. If Squid does something
else here, that would be a pretty big bug IMO!


> I would like to change
> the behaviour so that a -i flag makes the whole acl case-insensitive;
> this seems to me consistent with the behavior documented in squid.conf
> which does not document the case of multiple -i flags in a single
> regex-type ACL. I'd like feedback on this proposal.

I am not 100% sure what you mean by "whole acl" exactly, but please see
the "Where do ACL flags belong?" RFC[1] for a comprehensive coverage of
a similar problem and the agreed solution. The initial discussion was
mostly noise, but things started to make sense around [2]. Your -i is
mentioned frequently, including the last(?) email on that thread[3].

AFAICT, the agreed solutions have not been implemented yet so if you
have the cycles to work on this, you may be able to address all the
related problems at once and make things consistent across all options
of all ACLs. You should probably post a revised RFC (summarizing all
known issues and corresponding changes) if you are going to do that.


[1]
http://lists.squid-cache.org/pipermail/squid-dev/2015-December/004034.html

[2]
http://lists.squid-cache.org/pipermail/squid-dev/2015-December/004044.html

[3]
http://lists.squid-cache.org/pipermail/squid-dev/2015-December/004096.html


Thank you,

Alex.