Re: [squid-dev] [PATCH] Refactor wordlist to SBufList in acl/RegexData

2016-10-27 Thread Kinkie 
On Thu, Oct 27, 2016 at 5:10 AM, Alex Rousskov
 wrote:
> On 10/26/2016 05:18 PM, Kinkie wrote:
 the attached patch refactors the use of wordlist to SBufList in
 acl/RegexData.cc
>
>> -while (wl != NULL) {
>> +for (SBuf i : sl) {
>
> If possible, please avoid creating new SBufs by declaring "i" to be a
> constant reference to SBuf. It is probably possible unless you modify
> "i" -- I cannot tell for sure by looking at all the "i"s in the patch.

SBuf::c_str() is unfortunately non-const which limits applicability.

> Renaming "i" to something longer and more informative like
> "patternOrOption" or even "slowPatternOrOption" would be useful for
> those who try to understand what is going on and/or have to find that
> variable in a text full of other "i"s.

Done, renamed to configurationLineWord.


>> +for (auto i : sl) {
>
> Same here, on all counts. Auto does not automatically adds "&" or "const".

I know, but this can't be changed due to c_str() below.

> What is the time difference in configuring, for example, one million of
> regexes using patched and unpatched code?

The results were so surprising I had to double-check correctness which
is confirmed.
For a 10-lines regex acl, "squid -k parse" time went DOWN from 36
seconds to 0.7 on my Intel Core i5 Macbook air.
By checking SBuf stats in cachemgr, the calling code is actually quite
bad for SBuf allocations; SBufs are apparently mostly used to hold
temporary c-strings and are immediately freed. I suspect however
calling code some O(n^2) or worse behavior in wordlist, hence the big
win.

> I have not reviewed the patch closely. I have no objections.

Thanks for the insights you gave.

Merging the updated code.

-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Refactor wordlist to SBufList in acl/RegexData

2016-10-26 Thread Alex Rousskov 
On 10/26/2016 05:18 PM, Kinkie wrote:
>>> the attached patch refactors the use of wordlist to SBufList in
>>> acl/RegexData.cc

> -while (wl != NULL) {
> +for (SBuf i : sl) {

If possible, please avoid creating new SBufs by declaring "i" to be a
constant reference to SBuf. It is probably possible unless you modify
"i" -- I cannot tell for sure by looking at all the "i"s in the patch.

Renaming "i" to something longer and more informative like
"patternOrOption" or even "slowPatternOrOption" would be useful for
those who try to understand what is going on and/or have to find that
variable in a text full of other "i"s.


> +for (auto i : sl) {

Same here, on all counts. Auto does not automatically adds "&" or "const".


What is the time difference in configuring, for example, one million of
regexes using patched and unpatched code?


I have not reviewed the patch closely. I have no objections.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Refactor wordlist to SBufList in acl/RegexData

2016-10-26 Thread Kinkie 
On Wed, Oct 26, 2016 at 10:40 PM, Amos Jeffries  wrote:
> On 27/10/2016 9:16 a.m., Kinkie wrote:
>> Hi all,
>>the attached patch refactors the use of wordlist to SBufList in
>> acl/RegexData.cc; the "test.txt" attachment shows the output of
>> "./src/squid -X 2>&1 | grep 'RegexData' >/tmp/test_plan.txt" when
>> applied to a default configuration file with these additions:
>> acl t1 dstdom_regex pattern1 pattern2
>> acl t2 dstdom_regex -i pattern3 pattern4
>> acl t3 dstdom_regex +i pattern5 pattern6
>>
>
> Audit:
>
> * can you please take advantage of the change to get rid of the
> "compileOptimisedREs: compileOptimisedREs:" in the logs

Cleaned logs up.

> * please use emplace_back instead of sl.push_back(SBuf(clean));
>  - line ~234

Ok. Fixed in all instances.

Updated patch attached. If noone objects before then, I'll merge in ~8hrs.


Francesco Chemolli


wordlist-sbuflist-aclregexdata.patch
Description: Binary data
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Refactor wordlist to SBufList in acl/RegexData

2016-10-26 Thread Amos Jeffries 
On 27/10/2016 9:16 a.m., Kinkie wrote:
> Hi all,
>the attached patch refactors the use of wordlist to SBufList in
> acl/RegexData.cc; the "test.txt" attachment shows the output of
> "./src/squid -X 2>&1 | grep 'RegexData' >/tmp/test_plan.txt" when
> applied to a default configuration file with these additions:
> acl t1 dstdom_regex pattern1 pattern2
> acl t2 dstdom_regex -i pattern3 pattern4
> acl t3 dstdom_regex +i pattern5 pattern6
> 

Audit:

* can you please take advantage of the change to get rid of the
"compileOptimisedREs: compileOptimisedREs:" in the logs

* please use emplace_back instead of sl.push_back(SBuf(clean));
 - line ~234


+1.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [PATCH] Refactor wordlist to SBufList in acl/RegexData

2016-10-26 Thread Kinkie 
Hi all,
   the attached patch refactors the use of wordlist to SBufList in
acl/RegexData.cc; the "test.txt" attachment shows the output of
"./src/squid -X 2>&1 | grep 'RegexData' >/tmp/test_plan.txt" when
applied to a default configuration file with these additions:
acl t1 dstdom_regex pattern1 pattern2
acl t2 dstdom_regex -i pattern3 pattern4
acl t3 dstdom_regex +i pattern5 pattern6



-- 
Francesco Chemolli
2016/10/26 21:07:27.313| 28,2| RegexData.cc(228) parse: new Regex line or file
2016/10/26 21:07:27.313| 28,3| RegexData.cc(237) parse: buffering RE '-i'
2016/10/26 21:07:27.313| 28,3| RegexData.cc(237) parse: buffering RE 
'^cache_object://'
2016/10/26 21:07:27.313| 28,3| RegexData.cc(237) parse: buffering RE '+i'
2016/10/26 21:07:27.314| 28,3| RegexData.cc(237) parse: buffering RE 
'^https?://[^/]+/squid-internal-mgr/'
2016/10/26 21:07:27.314| 28,2| RegexData.cc(151) compileOptimisedREs: 
compileOptimisedREs: -i
2016/10/26 21:07:27.314| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE '^cache_object://'
2016/10/26 21:07:27.314| 28,2| RegexData.cc(162) compileOptimisedREs: 
compileOptimisedREs: +i
2016/10/26 21:07:27.314| 28,2| RegexData.cc(118) compileRE: compiled 
'(^cache_object://)' with flags 7
2016/10/26 21:07:27.315| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE '^https?://[^/]+/squid-internal-mgr/'
2016/10/26 21:07:27.315| 28,2| RegexData.cc(118) compileRE: compiled 
'(^https?://[^/]+/squid-internal-mgr/)' with flags 5
2016/10/26 21:07:27.315| 28,2| RegexData.cc(197) compileOptimisedREs: 
compileOptimisedREs: 2 REs are optimised into one RE.
2016/10/26 21:07:27.337| 28,2| RegexData.cc(228) parse: new Regex line or file
2016/10/26 21:07:27.337| 28,3| RegexData.cc(237) parse: buffering RE 'pattern1'
2016/10/26 21:07:27.337| 28,3| RegexData.cc(237) parse: buffering RE 'pattern2'
2016/10/26 21:07:27.337| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE 'pattern1'
2016/10/26 21:07:27.338| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE 'pattern2'
2016/10/26 21:07:27.338| 28,2| RegexData.cc(118) compileRE: compiled 
'(pattern1)|(pattern2)' with flags 5
2016/10/26 21:07:27.338| 28,2| RegexData.cc(197) compileOptimisedREs: 
compileOptimisedREs: 2 REs are optimised into one RE.
2016/10/26 21:07:27.338| 28,2| RegexData.cc(228) parse: new Regex line or file
2016/10/26 21:07:27.338| 28,3| RegexData.cc(237) parse: buffering RE '-i'
2016/10/26 21:07:27.338| 28,3| RegexData.cc(237) parse: buffering RE 'pattern3'
2016/10/26 21:07:27.338| 28,3| RegexData.cc(237) parse: buffering RE 'pattern4'
2016/10/26 21:07:27.338| 28,2| RegexData.cc(151) compileOptimisedREs: 
compileOptimisedREs: -i
2016/10/26 21:07:27.338| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE 'pattern3'
2016/10/26 21:07:27.338| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE 'pattern4'
2016/10/26 21:07:27.338| 28,2| RegexData.cc(118) compileRE: compiled 
'(pattern3)|(pattern4)' with flags 7
2016/10/26 21:07:27.338| 28,2| RegexData.cc(197) compileOptimisedREs: 
compileOptimisedREs: 2 REs are optimised into one RE.
2016/10/26 21:07:27.338| 28,2| RegexData.cc(228) parse: new Regex line or file
2016/10/26 21:07:27.338| 28,3| RegexData.cc(237) parse: buffering RE '+i'
2016/10/26 21:07:27.338| 28,3| RegexData.cc(237) parse: buffering RE 'pattern5'
2016/10/26 21:07:27.338| 28,3| RegexData.cc(237) parse: buffering RE 'pattern6'
2016/10/26 21:07:27.339| 28,2| RegexData.cc(160) compileOptimisedREs: 
compileOptimisedREs: optimisation of +i ... +i
2016/10/26 21:07:27.339| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE 'pattern5'
2016/10/26 21:07:27.339| 28,2| RegexData.cc(169) compileOptimisedREs: 
compileOptimisedREs: adding RE 'pattern6'
2016/10/26 21:07:27.339| 28,2| RegexData.cc(118) compileRE: compiled 
'(pattern5)|(pattern6)' with flags 5
2016/10/26 21:07:27.339| 28,2| RegexData.cc(197) compileOptimisedREs: 
compileOptimisedREs: 2 REs are optimised into one RE.


wordlist-sbuflist-aclregexdata.patch
Description: Binary data
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev