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

[Amos Jeffries]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/01/2015 11:19 p.m., Kinkie wrote:
> On Tue, Jan 13, 2015 at 3:09 AM, Amos Jeffries wrote:
>> 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/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.

We keep saying that (me too sometimes) about the paths which are not
commonly used. However the config parsing pathways are used on a -k
reconfigure event. Which is rare-ish but critical since the service is
actually suspended for the duration those pathways take to process the
config.

So yeah this is a performance critical pathway (when its used). Just
not a common one.

IMO we should be optimizing these paths as much as we can reasonably
do so when we do touch them, but not going out of our way to trade
complexity against speed like for the more commonly used code paths.


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

It seems to work when its been used elsewhere. Apparently the compiler
makes a choice whether to "unroll" that part of the ctor/dtor sequence
or make it a static call. That choice may vary depending on
optimization level and the .cc code around where its used.

So we gain in simpler code, and occasionally in faster code though not
reliably on the latter.

> 
>> * 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?
> 

Up to you.

Amos
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)

iQEcBAEBAgAGBQJUtPWPAAoJELJo5wb/XPRjPm4IANkKZUr02PVsb33CEyj1XDbF
yHEK7LS+NW17YkuE9phh1dh1XRJEQZOdrKUT4+7nsQvLjF/SLetwpyZKHSbAsCEk
PGSPNt47r2uaurNWH0RGxDC8EhDraRf9LpmXsLuI7Bq8AX1HpNLXrJuWBbtNXn89
LS6Nk2raDPumzflBPnWz8MoWSI5goYrFmQQAnFznIzIn196SGpCXDtk6N1qmSxin
Y00Tht6FFOgxfZ+WVz/PHWWpYVSZ6GEfIAE4Rmlfu1GmwhOPjrW8f6kCmrq7fXUQ
abaqEAfP9Wq727KeD6NtrgOoXhiMluWVX6pl4UOOv2/vafsPLvqB495hboooKc4=
=cN0T
-----END PGP SIGNATURE-----