[squid-dev] [PATCH] Coverity(-inspired) fixes part four, HttpHeader refactor

[Alex Rousskov]

On 08/21/2015 04:37 AM, Kinkie wrote:

>   the attached patch does:

... a long list of mostly unrelated changes ...

Sigh. Some of the patch changes are not necessary at all. Some are
useful. Some should be redone to minimize conflicts. Some are
performance optimizations that should be considered very carefully. It
would take too much time to sift through all of this and separate the
good from the bad, so we would have to accept pretty much all of them,
create a lot of conflicts, and probably miss bugs.

Sometimes, changing a lot of code is necessary. There is no way around
that. However, combining many unrelated changes, some of which change a
lot of code, and some of which are performance-sensitive or bug-prone,
is virtually never necessary.


> -        Tolower(name);
> -
> -        if (strcmp(name, "*") == 0) {
> -            /* Can not handle "Vary: *" withtout ETag support */
> -            safe_free(name);
> +        SBuf name(item, ilen);
> +        if (name.cmp("*",1) == 0) {
>              vstr.clean();
>              break;
>          }
> -
> -        strListAdd(&vstr, name, ',');
> +        name.toLower();

Do you know why we need to convert header names to lowercase here? The
header names are not a part of some Store hash, right? This conversion
is expensive because most header names will be changed here, breaking
SBuf reuse. If you know the reason, let's add a comment since you are
changing this code anyway.


>     in src/http.cc:
> 
>     * please mark use of name.c_str() with "XXX: performance regression,
>     c_str() reallocates"
> 
> 
> Actually, no in this case it doesn't unless ilen is exactly equal to one
> of the MemBlob MemPool sizes (possible but unlikely, I estimate in the
> 0.1% range).

*If* we remove toLower(), then there will be other, very common cases
where c_str() will reallocate in this context, either right now or when
the surrounding code is optimized to be more SBuf-friendly. Without
toLower(), the "name" SBuf should be pointing in the middle of another
SBuf in most cases where name was already in lower case.

I would honor Amos request for an XXX. In that XXX, I would not use the
word "regression" because this change may not make performance worse
than it was, but it is still bad code from performance point of view and
should eventually be changed/optimized. I would write something like:

  // XXX: will reallocate if toLower() above is removed


> -        if (strcmp(name, "*") == 0) {
> -            /* Can not handle "Vary: *" withtout ETag support */
> -            safe_free(name);
> +        SBuf name(item, ilen);
> +        if (name.cmp("*",1) == 0) {

AFAICT, the new code looks for names starting with "*". The old code was
looking for "*" names.


> -    while ((e = getEntry(&pos))) {
> -        if (e->id == id)
> -            delAt(pos, count);
> -    }
> +    int sz_before = entries.size();
> +    entries.erase(std::remove_if(entries.begin(), entries.end(),
> +    [=](HttpHeaderEntry *e) { return e && e->id == id; }),
> +    entries.end());

This change is a performance regression AFAICT: Unlike the old delAt(),
std::remove_if is very expensive for std::vector. Consider using
std::replace_if without .erasing instead.


> +enum HdrKind {

Use C++11 class enums for global enums, especially when such use does
not increase the amount of old code changes. In this specific case, it
does not AFAICT.


> -    for(HttpHeaderEntry *e : entries) {
> +    for (HttpHeaderEntry *e : entries) {

> -
> -    return NULL;        /* not reached */
> +    return nullptr;        /* not reached */

>      /* check mask first */
> -
>      if (!CBIT_TEST(mask, id))
>          return NULL;

...

Avoid out of scope changes, especially whitespace changes and NULL renaming.


> +    assert(0);

s/0/false/


> +    int sz_before = entries.size();
> +    entries.erase(std::remove_if(entries.begin(), entries.end(),
> +    [=](HttpHeaderEntry *e) { return e && e->id == id; }),
> +    entries.end());
> +    int count = entries.size() - sz_before;

If you can make sz_before, *e, and/or count const, make them const.


> +        if (e != nullptr && e->id == id)

You can save a lot of ink by writing "e" instead of "e != nullptr".


As always, please post patches with at least 20 context lines to
increase chances of reviewers spotting bugs.


Alex.