<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Aug 23, 2015 at 7:55 PM, Alex Rousskov <span dir="ltr"><<a href="mailto:rousskov@measurement-factory.com" target="_blank">rousskov@measurement-factory.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 08/21/2015 11:31 AM, Kinkie wrote:<br>
<span class="">> On Fri, Aug 21, 2015 at 6:39 PM, Alex Rousskov wrote:<br>
><br>
> On 08/21/2015 04:37 AM, Kinkie wrote:<br>
><br>
> > the attached patch does:<br>
><br>
> ... a long list of mostly unrelated changes ...<br>
><br>
> Sigh. Some of the patch changes are not necessary at all. Some are<br>
> useful. Some should be redone to minimize conflicts. Some are<br>
> performance optimizations that should be considered very carefully. It<br>
> would take too much time to sift through all of this and separate the<br>
> good from the bad, so we would have to accept pretty much all of them,<br>
> create a lot of conflicts, and probably miss bugs.<br>
<br>
<br>
</span>> To defend the code, ...<br>
<br>
Your code was not under attack. Your choice to assemble many unrelated<br>
and/or supposed-to-be-sequential changes into one patch was.<br></blockquote><div><br></div><div>I'm sorry about that. I tend to go depth-first, unfortunately.<br></div><div>I'll try to resist, but it's really hard for me.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
> > - Tolower(name);<br>
> > -<br>
> > - if (strcmp(name, "*") == 0) {<br>
> > - /* Can not handle "Vary: *" withtout ETag support */<br>
> > - safe_free(name);<br>
> > + SBuf name(item, ilen);<br>
> > + if (name.cmp("*",1) == 0) {<br>
> > vstr.clean();<br>
> > break;<br>
> > }<br>
> > -<br>
> > - strListAdd(&vstr, name, ',');<br>
> > + name.toLower();<br>
><br>
> Do you know why we need to convert header names to lowercase here? The<br>
> header names are not a part of some Store hash, right? This conversion<br>
> is expensive because most header names will be changed here, breaking<br>
> SBuf reuse. If you know the reason, let's add a comment since you are<br>
> changing this code anyway.<br>
<br>
<br>
</span><span class="">> Since lowerCase("*") == "*", I'd hope that the behavior is unchanged,<br>
<br>
</span>I did not say something has changed. I asked *why* we need to convert<br>
header names to lower case. I am not trying to sneak in some veiled<br>
attack on your code inside an innocent-looking question. I asked exactly<br>
what I wanted to know. An "I do not know" would be a perfectly<br>
acceptable response, of course, but if you do know, it would be good to<br>
add a comment.<br></blockquote><div><br></div><div>You are right, I got defensive over nothing.<br></div><div>Truth is, I don't know.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
<br>
> // XXX: will reallocate if toLower() above is removed<br>
><br>
><br>
> .. and strListGetItem will start emitting SBufs in place of (char*,<br>
> length).<br>
<br>
</span>Right, so perhaps s/will/may/ to be less precise since there are many<br>
factors here. The toLower() call is the most hidden factor though. This<br>
is not important -- if you disagree that a comment would be useful, do<br>
not add it. This is not an attack on your code.<br></blockquote><div><br></div><div>Adding comment: "will reallocate if ilen happens to equal pool size"<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<span class=""><br>
<br>
> > - while ((e = getEntry(&pos))) {<br>
> > - if (e->id == id)<br>
> > - delAt(pos, count);<br>
> > - }<br>
> > + int sz_before = entries.size();<br>
> > + entries.erase(std::remove_if(entries.begin(), entries.end(),<br>
> > + [=](HttpHeaderEntry *e) { return e && e->id == id; }),<br>
> > + entries.end());<br>
><br>
> This change is a performance regression AFAICT: Unlike the old delAt(),<br>
> std::remove_if is very expensive for std::vector. Consider using<br>
> std::replace_if without .erasing instead.<br>
><br>
><br>
> yes, it is a small regresson (from N to 2N)<br>
<br>
</span>I suspect a factor of 2 does not describe the overhead accurately. If<br>
remove_if removes a field, then all subsequent fields (except those that<br>
are also removed) would have to be moved, which is probably much more<br>
expensive than just checking their values twice.<br>
<span class=""><br>
<br>
> ). Reimplemented using replace_if - readability goes a bit down the<br>
> drain, unfortunatley.<br>
<br>
</span>Perhaps a little bit. Personally, I could not interpret the earlier<br>
erase(remove_if) combination without looking things up anyway :-).<br>
</div><span class=""><br></span></blockquote><div><br></div><div>This is what it looks like right now:<br><br> //set all items in entries where item is not nullptr and <br> // has same id as argument, to nullptr. Increment count variable<br> // as a side-effect on matches during sweep<br> std::replace_if(entries.begin(), entries.end(),<br> [&](HttpHeaderEntry *e) {<br> if (e && e->id == id) {<br> ++count;<br> return true;<br> }<br> return false;<br> },<br> nullptr);<br><br></div><div>Hopefully the comment will clarify intent :)<br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><span class="">
> > +enum HdrKind {<br>
><br>
> Use C++11 class enums for global enums, especially when such use does<br>
> not increase the amount of old code changes. In this specific case, it<br>
> does not AFAICT.<br>
><br>
><br>
> Unfortunately it's not possible in ordeer to use it as a bit-field (I<br>
> have tried and the compiler refused)<br>
<br>
</span>Interesting! If you still have an example of what goes wrong, can you<br>
post it? I tried a few tests myself, but they seem to be working fine if<br>
I specify the underlying class storage type (as we should, per [1]):<br></div></blockquote><div><br></div><div>Using clang on MacOS:<br></div><div>------------------------ source code<br></div><div>#include <iostream><br><br>enum class foo : std::int8_t {<br> zero = 0,<br> one = 1,<br> two = 1<<1,<br> three = 1<<2,<br> four = 1<<3<br>};<br><br>std::int8_t var = foo::one | foo::four;<br><br>int main() {<br> std::cout << var << std::endl;<br> return 0;<br>}<br></div><div>---------------------------- compiler output <br></div><div>example.cc:11:28: error: invalid operands to binary expression ('foo' and 'foo')<br>std::int8_t var = foo::one | foo::four;<br> ~~~~~~~~ ^ ~~~~~~~~~<br>1 error generated.<br></div><div> -----------------------------</div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><span class="">
<br>
<br>
> "bzr send" unfortunately doesn't allow to specify context to be added :(<br>
<br>
</span>Yes, that is unfortunate indeed. Fortunately, "bzr send" is not the only<br>
way to post a patch for review :-).<br></div></blockquote><div><br></div><div>Yes :)<br></div><div>Attached current patch. <br></div><div>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).<br><br></div><div>Thanks!<br clear="all"></div></div><br>-- <br><div class="gmail_signature"> Francesco</div>
</div></div>