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

[Kinkie]

On Mon, Nov 14, 2016 at 5:30 AM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> 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)?

If we feed too big a regex to the regex compiler it will crash with
OOM; 1024 bytes is probably on the conservative side, but it's hard to
set a predictable limit so I've mostly ketp it. This said, I've
slightly raised the limit by not counting the separators in the total
length, and this explains the difference: we use a few more patterns
per optimized regex.

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

Now that is interesting. The code is apparently buggy, and will forget
the last (or first) acl for each optimized ACL. I suspect we have a
bug in trunk, and have had for some time.

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

I've done this test. Whatever performance regression this is
introducing is more than offset by the big benefit of dumping wordlist
in fovour of sbuflist.
I've tested on a 1M-entries, 13MB-long acl, where entries are randomly
generated:

Time for "squid -k parse" was  23 seconds on my macbook air.

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

Amos already answered this one.

>> [RFC]

Thanks for the pointers on this; I'll check them and move to a
separate thread as you suggest.

  Kinkie