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. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
