On 11/04/2016 01:12 AM, Kinkie wrote:
> On Thu, Nov 3, 2016 at 10:55 PM, Alex Rousskov
> <[email protected]> wrote:
>> On 11/03/2016 03:19 PM, Kinkie wrote:
>>> On Tue, Nov 1, 2016 at 8:47 PM, Alex Rousskov wrote:
>>>> On 11/01/2016 02:02 PM, Kinkie wrote:
>>>>> the attached patch extends SBufContainerJoin to have prefix and
>>>>> suffix arguments.
>>
>>>> I recommend reworking this by adding a dest parameter:
>>>> SBuf &ContainerToSBuf(SBuf &dest, ...);
> attached v3 does this.
> SBuf
> +JoinContainerIntoSBuf(SBuf &dest, ...
Please return dest, not a copy of it. There is no reason to create and
destroy a new SBuf object here IMO. SBuf creation and destruction are
not as expensive as underlying storage copying, but they are still a
measurable expense without justification AFAICT.
There are certainly many cases where using references is a bad idea but
returning a fed-by-reference "dest" is not one of them AFAICT. In fact,
it is nearly the norm in "chainable" calls such as those that append
something.
> +/// variant of JoinContainerIntoSBuf not needing a SBuf lvalue
I recommend a more informative description than retelling of the
function declaration code. For example:
/// convenience JoinContainerIntoSBuf() wrapper with an extra memory
allocation
> + dest.reserveSpace(sz + prefix.length() + suffix.length());
I would reorder the operands to make the code more self-documenting,
especially when using an anonymous variable name like "sz":
dest.reserveSpace(prefix.length() + sz + suffix.length());
> I am not generally worried as it seems you are about copying SBufs as
> long as the underlying storage is not reallocated needlessly.
Unfortunately, all three patch versions reallocate storage needlessly
(for various reasons). You can study the attached debugging from the
attached patch to see what is actually going on in various
JoinContainerIntoSBuf implementations. It may surprise and inspire you
to make the code better. It did surprise and inspire me (but I wish I
was not doing this legwork!).
Thank you,
Alex.
2016/11/04 11:47:21.841| test starts
2016/11/04 11:47:21.841| 24,8| SBuf.cc(42) SBuf: SBuf2698 created
2016/11/04 11:47:21.841| 24,8| SBuf.cc(908) cow: SBuf2698 new size:1024
2016/11/04 11:47:21.841| 24,8| SBuf.cc(878) reAlloc: SBuf2698 new size: 1024
2016/11/04 11:47:21.841| 24,9| MemBlob.cc(56) MemBlob: constructed, this=0x202eb70 id=blob1484 reserveSize=1024
2016/11/04 11:47:21.841| 24,8| MemBlob.cc(101) memAlloc: blob1484 memAlloc: requested=1024, received=1024
2016/11/04 11:47:21.841| 24,7| SBuf.cc(887) reAlloc: SBuf2698 new store capacity: 1024
2016/11/04 11:47:21.841| test appended #0:
2016/11/04 11:47:21.841| 24,7| SBuf.cc(202) append: from c-string to id SBuf2698
2016/11/04 11:47:21.841| 24,7| SBuf.cc(153) rawSpace: reserving 5 for SBuf2698
2016/11/04 11:47:21.841| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(32) JoinContainerIntoSBuf1: started
2016/11/04 11:47:21.841| 24,8| SBuf.cc(42) SBuf: SBuf2699 created
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(34) JoinContainerIntoSBuf1: created empty buf
2016/11/04 11:47:21.841| 24,8| SBuf.cc(908) cow: SBuf2699 new size:100
2016/11/04 11:47:21.841| 24,8| SBuf.cc(878) reAlloc: SBuf2699 new size: 100
2016/11/04 11:47:21.841| 24,9| MemBlob.cc(56) MemBlob: constructed, this=0x202e9f0 id=blob1485 reserveSize=100
2016/11/04 11:47:21.841| 24,8| MemBlob.cc(101) memAlloc: blob1485 memAlloc: requested=100, received=128
2016/11/04 11:47:21.841| 24,7| SBuf.cc(887) reAlloc: SBuf2699 new store capacity: 128
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(36) JoinContainerIntoSBuf1: reserved space
2016/11/04 11:47:21.841| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2699
2016/11/04 11:47:21.841| 24,7| SBuf.cc(160) rawSpace: SBuf2699 not growing
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(38) JoinContainerIntoSBuf1: appended first item
2016/11/04 11:47:21.841| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2699
2016/11/04 11:47:21.841| 24,7| SBuf.cc(160) rawSpace: SBuf2699 not growing
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(40) JoinContainerIntoSBuf1: appended second item
2016/11/04 11:47:21.841| 24,7| SBuf.cc(153) rawSpace: reserving 14 for SBuf2698
2016/11/04 11:47:21.841| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.841| 24,8| SBuf.cc(85) ~SBuf: SBuf2699 destructed
2016/11/04 11:47:21.841| 24,9| MemBlob.cc(82) ~MemBlob: destructed, this=0x202e9f0 id=blob1485 capacity=128 size=14
2016/11/04 11:47:21.841| test appended #1: Foo: value1 value1
2016/11/04 11:47:21.841| 24,7| SBuf.cc(202) append: from c-string to id SBuf2698
2016/11/04 11:47:21.841| 24,7| SBuf.cc(153) rawSpace: reserving 5 for SBuf2698
2016/11/04 11:47:21.841| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(47) JoinContainerIntoSBuf2: started
2016/11/04 11:47:21.841| 1,2| AccessLogEntry.cc(48) JoinContainerIntoSBuf2: did not create an empty buf
2016/11/04 11:47:21.841| 24,8| SBuf.cc(908) cow: SBuf2698 new size:124
2016/11/04 11:47:21.841| 24,8| SBuf.cc(878) reAlloc: SBuf2698 new size: 124
2016/11/04 11:47:21.841| 24,9| MemBlob.cc(56) MemBlob: constructed, this=0x202e9f0 id=blob1486 reserveSize=124
2016/11/04 11:47:21.841| 24,8| MemBlob.cc(101) memAlloc: blob1486 memAlloc: requested=124, received=128
2016/11/04 11:47:21.841| 24,9| MemBlob.cc(82) ~MemBlob: destructed, this=0x202eb70 id=blob1484 capacity=1024 size=24
2016/11/04 11:47:21.842| 24,7| SBuf.cc(887) reAlloc: SBuf2698 new store capacity: 128
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(50) JoinContainerIntoSBuf2: reserved space
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(52) JoinContainerIntoSBuf2: appended first item
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(54) JoinContainerIntoSBuf2: appended second item
2016/11/04 11:47:21.842| 24,8| SBuf.cc(50) SBuf: SBuf2700 created from id SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 38 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 24,8| SBuf.cc(85) ~SBuf: SBuf2700 destructed
2016/11/04 11:47:21.842| test appended #2: Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2
2016/11/04 11:47:21.842| 24,7| SBuf.cc(202) append: from c-string to id SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 5 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(60) JoinContainerIntoSBuf3: started
2016/11/04 11:47:21.842| 24,8| SBuf.cc(908) cow: SBuf2698 new size:181
2016/11/04 11:47:21.842| 24,8| SBuf.cc(878) reAlloc: SBuf2698 new size: 181
2016/11/04 11:47:21.842| 24,9| MemBlob.cc(56) MemBlob: constructed, this=0x202eb70 id=blob1487 reserveSize=181
2016/11/04 11:47:21.842| 24,8| MemBlob.cc(101) memAlloc: blob1487 memAlloc: requested=181, received=512
2016/11/04 11:47:21.842| 24,9| MemBlob.cc(82) ~MemBlob: destructed, this=0x202e9f0 id=blob1486 capacity=128 size=81
2016/11/04 11:47:21.842| 24,7| SBuf.cc(887) reAlloc: SBuf2698 new store capacity: 512
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(62) JoinContainerIntoSBuf3: reserved space
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(64) JoinContainerIntoSBuf3: appended first item
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(66) JoinContainerIntoSBuf3: appended second item
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 95 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| test appended #3: Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2 Baz: value3 value3 Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2 Baz: value3 value3
2016/11/04 11:47:21.842| 24,7| SBuf.cc(202) append: from c-string to id SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 5 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(72) JoinContainerIntoSBuf4: started
2016/11/04 11:47:21.842| 24,8| SBuf.cc(130) reserve: SBuf2698 was: 0+195+317=512
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(77) JoinContainerIntoSBuf4: reserved space
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(79) JoinContainerIntoSBuf4: appended first item
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 7 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| 1,2| AccessLogEntry.cc(81) JoinContainerIntoSBuf4: appended second item
2016/11/04 11:47:21.842| 24,7| SBuf.cc(153) rawSpace: reserving 209 for SBuf2698
2016/11/04 11:47:21.842| 24,7| SBuf.cc(160) rawSpace: SBuf2698 not growing
2016/11/04 11:47:21.842| test appended #4: Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2 Baz: value3 value3 Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2 Baz: value3 value3 Boo: value4 value4 Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2 Baz: value3 value3 Foo: value1 value1 Bar: value2 value2 Foo: value1 value1 Bar: value2 value2 Baz: value3 value3 Boo: value4 value4
2016/11/04 11:47:21.842| 24,8| SBuf.cc(85) ~SBuf: SBuf2698 destructed
2016/11/04 11:47:21.842| 24,9| MemBlob.cc(82) ~MemBlob: destructed, this=0x202eb70 id=blob1487 capacity=512 size=418
2016/11/04 11:47:21.842| test ends
Testing allocations in various JoinContainerIntoSBuf() implementations.
=== modified file 'src/AccessLogEntry.cc'
--- src/AccessLogEntry.cc 2016-10-28 11:55:44 +0000
+++ src/AccessLogEntry.cc 2016-11-04 17:47:11 +0000
@@ -15,6 +15,73 @@
#if USE_OPENSSL
#include "ssl/support.h"
+static SBuf value1("value1 ");
+static SBuf value2("value2 ");
+static SBuf value3("value3 ");
+static SBuf value4("value4 ");
+
+void JoinContainerIntoSBufPrep()
+{
+ debugs(1,1, value1 << value2 << value3);
+}
+
+#define progress(where) debugs(1,2, where)
+
+SBuf JoinContainerIntoSBuf1()
+{
+ progress("started");
+ SBuf buf;
+ progress("created empty buf");
+ buf.reserveSpace(100);
+ progress("reserved space");
+ buf.append(value1);
+ progress("appended first item");
+ buf.append(value1);
+ progress("appended second item");
+ return buf;
+}
+
+
+SBuf JoinContainerIntoSBuf2(SBuf &buf)
+{
+ progress("started");
+ progress("did not create an empty buf");
+ buf.reserveSpace(100);
+ progress("reserved space");
+ buf.append(value2);
+ progress("appended first item");
+ buf.append(value2);
+ progress("appended second item");
+ return buf;
+}
+
+SBuf &JoinContainerIntoSBuf3(SBuf &buf)
+{
+ progress("started");
+ buf.reserveSpace(100);
+ progress("reserved space");
+ buf.append(value3);
+ progress("appended first item");
+ buf.append(value3);
+ progress("appended second item");
+ return buf;
+}
+
+SBuf &JoinContainerIntoSBuf4(SBuf &buf)
+{
+ progress("started");
+ SBufReservationRequirements spaceRequirements;
+ spaceRequirements.idealSpace = 100;
+ spaceRequirements.minSpace = 100;
+ buf.reserve(spaceRequirements);
+ progress("reserved space");
+ buf.append(value4);
+ progress("appended first item");
+ buf.append(value4);
+ progress("appended second item");
+ return buf;
+}
+
AccessLogEntry::SslDetails::SslDetails(): user(NULL), bumpMode(::Ssl::bumpEnd)
{
}
=== modified file 'src/main.cc'
--- src/main.cc 2016-10-03 04:33:08 +0000
+++ src/main.cc 2016-11-04 17:44:17 +0000
@@ -1408,6 +1408,12 @@
}
}
+extern void JoinContainerIntoSBufPrep();
+extern SBuf JoinContainerIntoSBuf1();
+extern SBuf JoinContainerIntoSBuf2(SBuf &buf);
+extern SBuf &JoinContainerIntoSBuf3(SBuf &buf);
+extern SBuf &JoinContainerIntoSBuf4(SBuf &buf);
+
int
SquidMain(int argc, char **argv)
{
@@ -1616,6 +1622,31 @@
#endif
+ JoinContainerIntoSBufPrep();
+ debugs(1,1, "test starts");
+ {
+ SBuf buffer;
+ buffer.reserveSpace(1024);
+ debugs(1,1, "test appended #0: " << buffer);
+
+ buffer.append("Foo: ", 5);
+ buffer.append(JoinContainerIntoSBuf1());
+ debugs(1,1, "test appended #1: " << buffer);
+
+ buffer.append("Bar: ", 5);
+ buffer.append(JoinContainerIntoSBuf2(buffer));
+ debugs(1,1, "test appended #2: " << buffer);
+
+ buffer.append("Baz: ", 5);
+ buffer.append(JoinContainerIntoSBuf3(buffer));
+ debugs(1,1, "test appended #3: " << buffer);
+
+ buffer.append("Boo: ", 5);
+ buffer.append(JoinContainerIntoSBuf4(buffer));
+ debugs(1,1, "test appended #4: " << buffer);
+ }
+ debugs(1,1, "test ends");
+
/* main loop */
EventLoop mainLoop;
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
