[squid-dev] [PATCH] cleanup external_acl_type cache

[Alex Rousskov]

On 04/11/2017 07:02 PM, Amos Jeffries wrote:
> This patch 

Please use much larger context for diffs. Adding the current function
name to hunks helps a lot as well:

> $ cat ~/.bazaar/bazaar.conf
...
> [ALIASES]
> diff = diff --diff-options='-U 30 --show-c-function'


> begins the refactoring of the external ACL lookup cache by
> replacing the LRU list of cached entries with a std::list. Removing one
> more use of dlinklist.

I have to vote -1 because of the following combination of factors:

* std::list is often a lot more expensive (performance-wise) than an
intrusive list like dlinklist

* this seems to be a performance-sensitive area

* no justification has been given to address the (obvious?) concerns
related to replacing an efficient intrusive list with std::list.


> -    dlinkDelete(&e->lru, &def->lru_list);
> +    def->lru_list.remove(entry);

For example, the above change replaces an O(1) dlinkDelete() with an
O(n) linear search by std::list::remove(). On a busy proxy, this code is
executed on every external ACL cache addition AFAICT.

As always, I am ready to change my vote if I misunderstood your work.


BTW, if you do end up removing this intrusive list, please check whether
its ExternalACLEntry::lru "anchor" member should be removed as a side
effect. Perhaps you already have checked that.


I may have an intrusive list template somewhere. Please let me know if
you want me to search for it (in case you would like to replace
dlink_list with an STL-like container while preserving performance).


Thank you,

Alex.