Re: Regression introduced in trunk r13201 (large rock merge)
I did bzr pool before diff. No changed files were found. Web interface also shows no modified files - http://bazaar.launchpad.net/~squid/squid/trunk/revision/13257 On Thu, Feb 6, 2014 at 8:24 AM, Kinkie gkin...@gmail.com wrote: It' the tip, a bzr pull should get it for you On Feb 6, 2014 2:45 AM, Nikolai Gorchilov n...@x3me.net wrote: Thanx, Alex, I'm ready to test it immediately, but unfortunately, due to unknown reason r13257 doesn't show any file modifications. I.e. bzr diff -r13256..13257 produces empty result. Best, Niki On Wed, Feb 5, 2014 at 8:49 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 02/03/2014 03:22 PM, Alex Rousskov wrote: On 02/03/2014 01:55 PM, Amos Jeffries wrote: FWIW; this seems to be the same issue is under discussion in squid-users thread rock store: a bug or ...?. Cc'ing Henrik Lidström and Nikolai Gorchilov. Henrik, Nokolai: if you could followup to squid-dev in future about this please. On 2014-02-04 05:25, Kinkie wrote: Hi, it appears that the large rock merge has introduced a regression, probably related to the handling of keepalive and/or about finishing to send an object to a client. I have confirmed that the issue is NOT present in revno 13200, but it is present in r13203 (r13201 and r13202 do not compile). FWIW, if nobody volunteers earlier, I should be able to look at this on Wednesday. FYI: Kinkie and I fixed the bug we could reproduce. See trunk r13257. The bug was a side-effect of the following Collapsed Forwarding change: revno: 12501.1.50 committer: Alex Rousskov rouss...@measurement-factory.com branch nick: collapsed-fwd timestamp: Tue 2013-06-25 09:39:10 -0600 message: Mark client streams that sent everything as STREAM_COMPLETE. The old code used STREAM_UNPLANNED_COMPLETE if the completed stream was associated with a non-persistent connection, which did not make sense to me and, IIRC, led to store entry aborts even though the entries were not damaged in any way. This change may expose other subtle bugs, but none are known at this time. See also: http://www.squid-cache.org/mail-archive/squid-dev/200702/0017.html http://www.squid-cache.org/mail-archive/squid-dev/201102/0210.html Cheers, Alex.
Re: Regression introduced in trunk r13201 (large rock merge)
Hi, Maybe there's some issue with launchpad mirroring? Can you try the branch at http://bzr.squid-cache.org/bzr/squid3/trunk ? On Thu, Feb 6, 2014 at 9:23 AM, Nikolai Gorchilov n...@x3me.net wrote: I did bzr pool before diff. No changed files were found. Web interface also shows no modified files - http://bazaar.launchpad.net/~squid/squid/trunk/revision/13257 On Thu, Feb 6, 2014 at 8:24 AM, Kinkie gkin...@gmail.com wrote: It' the tip, a bzr pull should get it for you On Feb 6, 2014 2:45 AM, Nikolai Gorchilov n...@x3me.net wrote: Thanx, Alex, I'm ready to test it immediately, but unfortunately, due to unknown reason r13257 doesn't show any file modifications. I.e. bzr diff -r13256..13257 produces empty result. Best, Niki On Wed, Feb 5, 2014 at 8:49 PM, Alex Rousskov rouss...@measurement-factory.com wrote: On 02/03/2014 03:22 PM, Alex Rousskov wrote: On 02/03/2014 01:55 PM, Amos Jeffries wrote: FWIW; this seems to be the same issue is under discussion in squid-users thread rock store: a bug or ...?. Cc'ing Henrik Lidström and Nikolai Gorchilov. Henrik, Nokolai: if you could followup to squid-dev in future about this please. On 2014-02-04 05:25, Kinkie wrote: Hi, it appears that the large rock merge has introduced a regression, probably related to the handling of keepalive and/or about finishing to send an object to a client. I have confirmed that the issue is NOT present in revno 13200, but it is present in r13203 (r13201 and r13202 do not compile). FWIW, if nobody volunteers earlier, I should be able to look at this on Wednesday. FYI: Kinkie and I fixed the bug we could reproduce. See trunk r13257. The bug was a side-effect of the following Collapsed Forwarding change: revno: 12501.1.50 committer: Alex Rousskov rouss...@measurement-factory.com branch nick: collapsed-fwd timestamp: Tue 2013-06-25 09:39:10 -0600 message: Mark client streams that sent everything as STREAM_COMPLETE. The old code used STREAM_UNPLANNED_COMPLETE if the completed stream was associated with a non-persistent connection, which did not make sense to me and, IIRC, led to store entry aborts even though the entries were not damaged in any way. This change may expose other subtle bugs, but none are known at this time. See also: http://www.squid-cache.org/mail-archive/squid-dev/200702/0017.html http://www.squid-cache.org/mail-archive/squid-dev/201102/0210.html Cheers, Alex. -- Francesco
Re: Regression introduced in trunk r13201 (large rock merge)
On Thu, Feb 6, 2014 at 10:54 AM, Kinkie gkin...@gmail.com wrote: Hi, Maybe there's some issue with launchpad mirroring? Can you try the branch at http://bzr.squid-cache.org/bzr/squid3/trunk ? Just tried with http://bzr.squid-cache.org/bzr/squid3/trunk - same empty diff as with launchpad mirror. Best, Niki
Re: Regression introduced in trunk r13201 (large rock merge)
Doh! you are right. For some reason I botched the commit in r13257. I've now committed the change as r13258. Please try it. On Thu, Feb 6, 2014 at 10:25 AM, Nikolai Gorchilov n...@x3me.net wrote: On Thu, Feb 6, 2014 at 10:54 AM, Kinkie gkin...@gmail.com wrote: Hi, Maybe there's some issue with launchpad mirroring? Can you try the branch at http://bzr.squid-cache.org/bzr/squid3/trunk ? Just tried with http://bzr.squid-cache.org/bzr/squid3/trunk - same empty diff as with launchpad mirror. Best, Niki -- Francesco
Re: Regression introduced in trunk r13201 (large rock merge)
On 6/02/2014 9:54 p.m., Kinkie wrote: Hi, Maybe there's some issue with launchpad mirroring? Can you try the branch at http://bzr.squid-cache.org/bzr/squid3/trunk ? I dont think so. The master server shows the same things on the checkout used to generate changesets and do maintenance. http://master.squid-cache.org/Versions/v3/3.HEAD/changesets/merge.html http://master.squid-cache.org/Versions/v3/3.HEAD/changesets/squid-3-13257.patch Amos
Re: Regression introduced in trunk r13201 (large rock merge)
Hi, Apparently my checked-out tree had some problems. I've performed a follow-up commit with the actual changes. On Thu, Feb 6, 2014 at 11:53 AM, Amos Jeffries squ...@treenet.co.nz wrote: On 6/02/2014 9:54 p.m., Kinkie wrote: Hi, Maybe there's some issue with launchpad mirroring? Can you try the branch at http://bzr.squid-cache.org/bzr/squid3/trunk ? I dont think so. The master server shows the same things on the checkout used to generate changesets and do maintenance. http://master.squid-cache.org/Versions/v3/3.HEAD/changesets/merge.html http://master.squid-cache.org/Versions/v3/3.HEAD/changesets/squid-3-13257.patch Amos -- Francesco
Re: [PATCH] refactor Vector
On 5/02/2014 8:57 a.m., Kinkie wrote:
On Sun, Feb 2, 2014 at 10:42 PM, Amos Jeffries squ...@treenet.co.nz wrote:
On 2014-02-03 08:06, Kinkie wrote:
Hi,
the attached patch (merge from lp:~squid/squid/vector-refactor) is
an attempt to refactor Vector and its clients so that:
- clients of Vector don't break layering
- the Vector API more closely matches std::vector
The eventual aim is to replace Vector with std::vector, only if some
more accurate measurements (in the lp:~squid/squid/vector-to-stdvector
branch) show that this wouldn't cause performance degradations.
Don't be fooled by the size of the patch; it is big because there's
lots of context.
Thanks
in src/HttpHdrRange.cc
* This pattern happens several times throughout this patch:
-while (!specs.empty())
-delete specs.pop_back();
+while (!specs.empty()) {
+delete specs.back();
+specs.pop_back();
+}
** std::vector::pop_back() is documented as erasing the element.
Squid::Vector does not.
- under correct usage of std::vector the loop and delete X.back() would
seem to be irrelevant.
** the pattern with loop seems to be equivalent to specs.clear() but far
less efficient. Efficiency is saved here only by Squid::Vector not
reallocating the items array in pop_back().
Both left in after Alex' comments.
Okay.
in src/HttpHeader.cc:
* please replace C-style casts with C++ casts in lines changed. static_cast
would be appropriate in several places in this patch, if needed at all.
Done.
in src/base/Vector.h
* still need to remove operator +=. There are likely more conversions to
push_back() hiding as a result.
Done. There were two.
v2 attached.
Okay. Looks good.
+1.
Amos
Re: Vector vs std::vector
On 01/30/2014 01:37 PM, Alex Rousskov wrote: On 01/30/2014 12:14 PM, Kinkie wrote: Ok, here's some numbers (same testing methodology as before) Trunk: mean RPS (CPU time) 10029.11 (996.661872) 9786.60 (1021.695007) 10116.93 (988.395665) 9958.71 (1004.039956) stdvector: mean RPS (CPUtime) 9732.57 (1027.426563) 10388.38 (962.418333) 10332.17 (967.824790) OK, so we are probably just looking at noise: The variation within each data set (1.4% or 3.6%) is about the same or more than the difference between the set averages (1.8%). We will test this in a Polygraph lab, but I would not be surprised to see no significant difference there as well. Polygraph results are in. No significant difference is visible. FWIW, here are a few basic response stats extracted from four tests (rep is reply and rptm is response time): * trunk vs. branch, take1: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880004.00 = 11880004.00 rep.rptm.mean: 0% 501.12 501.16 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880004.00 = 11880004.00 rep.size.mean: 0% 8738.48 8739.91 rep.size.std_dev: 0% 11446.90 11446.12 * trunk vs. branch, take2: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880008.00 = 11880002.00 rep.rptm.mean: 0% 501.14 501.16 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880008.00 = 11880002.00 rep.size.mean: 0% 8738.49 8739.14 rep.size.std_dev: 0% 11446.42 11448.64 The results appear to be pretty stable, but we have not run more/different tests to really verify that: * take1 vs. take2, trunk: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880004.00 = 11880008.00 rep.rptm.mean: 0% 501.12 501.14 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880004.00 = 11880008.00 rep.size.mean: 0% 8738.48 8738.49 rep.size.std_dev: 0% 11446.90 11446.42 * take1 vs. take2, branch: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880004.00 = 11880002.00 rep.rptm.mean: 0% 501.16 = 501.16 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880004.00 = 11880002.00 rep.size.mean: 0% 8739.91 8739.14 rep.size.std_dev: 0% 11446.12 11448.64 Note that the comparison script uses 1.0e-6 epsilon to declare equality so some slightly different numbers are printed with an equal sign. I hope I did not screw up while pasting these numbers :-). HTH, Alex.
Re: [PATCH] refactor Vector
On 02/04/2014 12:57 PM, Kinkie wrote:
On Sun, Feb 2, 2014 at 10:42 PM, Amos Jeffries squ...@treenet.co.nz wrote:
On 2014-02-03 08:06, Kinkie wrote:
Hi,
the attached patch (merge from lp:~squid/squid/vector-refactor) is
an attempt to refactor Vector and its clients so that:
- clients of Vector don't break layering
- the Vector API more closely matches std::vector
The eventual aim is to replace Vector with std::vector, only if some
more accurate measurements (in the lp:~squid/squid/vector-to-stdvector
branch) show that this wouldn't cause performance degradations.
v2 attached.
+VectorE::at(unsigned i)
{
assert (size() i);
return items[i];
}
templateclass E
const E
-VectorE::operator [] (unsigned i) const
+VectorE::at(unsigned i) const
{
assert (size() i);
return items[i];
}
Ideally, at() methods should be implemented using [] operators instead
of direct access to items, but that is very minor.
-ErrorDynamicPageInfo *info = ErrorDynamicPages.items[i -
ERR_MAX];
+ErrorDynamicPageInfo *info = ErrorDynamicPages[i - ERR_MAX];
This non-performance-critical line surrounded by relatively complex
index logic should use an at() method instead. Again a minor thing.
-theClient-start (tag + 1, (const char **)attributes.items,
attributes.size() 1);
+theClient-start (tag + 1, const_castconst char
**(attributes.data()), attributes.size() 1);
The kind of cast appears to be wrong here: Const_cast is normally used
to remove const, not add it. It is not used to change the type.
Also, .data() is a C++11 method. Should we avoid those for now? To be
compatible with older STLs, you may use the address of vector[0] instead
AFAICT.
All of the above can be addressed during commit IMO.
Thank you,
Alex.
Re: Vector vs std::vector
Thank you for running this test. I guess that with these results, it can make sense to go forward with the project of replacing Vector with std::vector, does everyone agree? Thanks! On Fri, Feb 7, 2014 at 6:18 AM, Alex Rousskov rouss...@measurement-factory.com wrote: On 01/30/2014 01:37 PM, Alex Rousskov wrote: On 01/30/2014 12:14 PM, Kinkie wrote: Ok, here's some numbers (same testing methodology as before) Trunk: mean RPS (CPU time) 10029.11 (996.661872) 9786.60 (1021.695007) 10116.93 (988.395665) 9958.71 (1004.039956) stdvector: mean RPS (CPUtime) 9732.57 (1027.426563) 10388.38 (962.418333) 10332.17 (967.824790) OK, so we are probably just looking at noise: The variation within each data set (1.4% or 3.6%) is about the same or more than the difference between the set averages (1.8%). We will test this in a Polygraph lab, but I would not be surprised to see no significant difference there as well. Polygraph results are in. No significant difference is visible. FWIW, here are a few basic response stats extracted from four tests (rep is reply and rptm is response time): * trunk vs. branch, take1: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880004.00 = 11880004.00 rep.rptm.mean: 0% 501.12 501.16 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880004.00 = 11880004.00 rep.size.mean: 0% 8738.48 8739.91 rep.size.std_dev: 0% 11446.90 11446.12 * trunk vs. branch, take2: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880008.00 = 11880002.00 rep.rptm.mean: 0% 501.14 501.16 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880008.00 = 11880002.00 rep.size.mean: 0% 8738.49 8739.14 rep.size.std_dev: 0% 11446.42 11448.64 The results appear to be pretty stable, but we have not run more/different tests to really verify that: * take1 vs. take2, trunk: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880004.00 = 11880008.00 rep.rptm.mean: 0% 501.12 501.14 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880004.00 = 11880008.00 rep.size.mean: 0% 8738.48 8738.49 rep.size.std_dev: 0% 11446.90 11446.42 * take1 vs. take2, branch: rep.rate: 0% 2199.97 = 2199.97 rep.rptm.count: 0% 11880004.00 = 11880002.00 rep.rptm.mean: 0% 501.16 = 501.16 rep.rptm.std_dev: 0% 485.77 = 485.77 rep.size.count: 0% 11880004.00 = 11880002.00 rep.size.mean: 0% 8739.91 8739.14 rep.size.std_dev: 0% 11446.12 11448.64 Note that the comparison script uses 1.0e-6 epsilon to declare equality so some slightly different numbers are printed with an equal sign. I hope I did not screw up while pasting these numbers :-). HTH, Alex. -- Francesco
