[squid-dev] [PATCH] Remove some splay users

[Kinkie]

Attached is the current patch in case it gets approved for an interim merge.

On Tue, Jan 13, 2015 at 11:19 AM, Kinkie <gkinkie at gmail.com> wrote:
> On Tue, Jan 13, 2015 at 3:09 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 13/01/2015 8:51 a.m., Kinkie wrote:
>>> Hello, the attached patch changes some users of splay to std::set.
>>> The aim is to get more predictable (if not necessarily better)
>>> performance and leverage a cleaner API resulting in more readable
>>> code.
>>>
>>> This is a merge snapshot of branch lp:~squid/squid/replace-splay
>>> revno 13835; I will continue working on the branch to migrate more
>>> users.
>>>
>>> The touched classes have been build- and run-tested.
>>>
>>
>>
>> in src/acl/UserData.h, src/acl/Eui64.h, etc
>> * still need to include splay.h in these .h files?
>>   perhapse it can be reduced down to the relevant .cc or dropped entirely.
>
> dropped.
>
>> in src/acl/Eui64.cc:
>> * ACLEui64::parse() can be optimized a bit further:
>>
>>    while (const char *t = ConfigParser::strtokFile()) {
>>     if (Eui::Eui64 *q = aclParseEuiData(t)) {
>>        ... what used to follow the if
>>        xfree(q);
>>     }
>
> done, although it is not a performance-critical path.
>
>>   }
>>
>> * ACLEui64::dump() buf variable can remain as static. It's only used
>> until SBuf data-copies, no need to re-allocate on each loop.
>
> Ok
>
>>  - also while changing, if possible use SZ_EUI64_BUF otherwise
>> document why magic number is 48 used. (EUI uses 3 readable characters
>> to represent each octet of the number - buf is approx. double the
>> required size).
>
> SZ_EUI64_BUF is 8; I understand that's for the binary representation,
> not textual.
> I kept the buffer as it was, after all it's not huge anyway.
>
>>
>>
>> in src/acl/UserData.cc:
>> * #include SbufAlgos.h before util.h
>
> Done.
>
>> * for ctor/dtor like ACLUserData::~ACLUserData() which have become
>> NOP, please inline them in the .h where possible. That allows some
>> more compiler optimizations.
>
> Is that even true when they are virtual? But sure, it means fewer
> lines of code so that's OK.
>
>> * is cend() usable instead of end() for the find test in the function
>> above ACLUserData::dump() [ chunk @55, I think its probably the end of
>> match()]
>
> Using const iterators in the copied-from container.
>
>> * why is CaseInsensitveSBufCompare() not defined in SBufAlgos.h for
>> re-use ?
>
> Should I do that as part of this branch or merge separately?



-- 
    Francesco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: replace-splay-v2.patch
Type: application/octet-stream
Size: 32463 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150113/6dc8204a/attachment-0001.obj>