On Sun, Aug 23, 2015 at 7:55 PM, Alex Rousskov < rouss...@measurement-factory.com> wrote:
> On 08/21/2015 11:31 AM, Kinkie wrote: > > On Fri, Aug 21, 2015 at 6:39 PM, Alex Rousskov wrote: > > > > 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. > > > > To defend the code, ... > > Your code was not under attack. Your choice to assemble many unrelated > and/or supposed-to-be-sequential changes into one patch was. > I'm sorry about that. I tend to go depth-first, unfortunately. I'll try to resist, but it's really hard for me. > > > - 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. > > > > Since lowerCase("*") == "*", I'd hope that the behavior is unchanged, > > I did not say something has changed. I asked *why* we need to convert > header names to lower case. I am not trying to sneak in some veiled > attack on your code inside an innocent-looking question. I asked exactly > what I wanted to know. An "I do not know" would be a perfectly > acceptable response, of course, but if you do know, it would be good to > add a comment. > You are right, I got defensive over nothing. Truth is, I don't know. > > > > // XXX: will reallocate if toLower() above is removed > > > > > > .. and strListGetItem will start emitting SBufs in place of (char*, > > length). > > Right, so perhaps s/will/may/ to be less precise since there are many > factors here. The toLower() call is the most hidden factor though. This > is not important -- if you disagree that a comment would be useful, do > not add it. This is not an attack on your code. > Adding comment: "will reallocate if ilen happens to equal pool size" > > > > > - 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. > > > > > > yes, it is a small regresson (from N to 2N) > > I suspect a factor of 2 does not describe the overhead accurately. If > remove_if removes a field, then all subsequent fields (except those that > are also removed) would have to be moved, which is probably much more > expensive than just checking their values twice. > > > > ). Reimplemented using replace_if - readability goes a bit down the > > drain, unfortunatley. > > Perhaps a little bit. Personally, I could not interpret the earlier > erase(remove_if) combination without looking things up anyway :-). > > This is what it looks like right now: //set all items in entries where item is not nullptr and // has same id as argument, to nullptr. Increment count variable // as a side-effect on matches during sweep std::replace_if(entries.begin(), entries.end(), [&](HttpHeaderEntry *e) { if (e && e->id == id) { ++count; return true; } return false; }, nullptr); Hopefully the comment will clarify intent :) > > > +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. > > > > > > Unfortunately it's not possible in ordeer to use it as a bit-field (I > > have tried and the compiler refused) > > Interesting! If you still have an example of what goes wrong, can you > post it? I tried a few tests myself, but they seem to be working fine if > I specify the underlying class storage type (as we should, per [1]): > Using clang on MacOS: ------------------------ source code #include <iostream> enum class foo : std::int8_t { zero = 0, one = 1, two = 1<<1, three = 1<<2, four = 1<<3 }; std::int8_t var = foo::one | foo::four; int main() { std::cout << var << std::endl; return 0; } ---------------------------- compiler output example.cc:11:28: error: invalid operands to binary expression ('foo' and 'foo') std::int8_t var = foo::one | foo::four; ~~~~~~~~ ^ ~~~~~~~~~ 1 error generated. ----------------------------- > > > > "bzr send" unfortunately doesn't allow to specify context to be added :( > > Yes, that is unfortunate indeed. Fortunately, "bzr send" is not the only > way to post a patch for review :-). > Yes :) Attached current patch. If I understand correclty, that the only point to clear before merge is how many lines away from a touched line should a HERE be to be removed (I'll try to come up with a script to do blanket HERE removal as said elsewhere in this thread). Thanks! -- Francesco
_______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev