Re: [squid-dev] [PATCH] Faster SBuf::append

[Alex Rousskov]

On 11/04/2016 08:22 AM, Amos Jeffries wrote:
> On 7/10/2016 6:20 a.m., Alex Rousskov wrote:
>> On 10/06/2016 10:57 AM, Amos Jeffries wrote:
>>
>>> Please add a check to the unit test testSBuf::testAppendSBuf()
>>> to guarantee that the (*this = S) assignment code path updates the store
>>> reference count rather than doing a bit-wise copy of the SBuf.
>>
>> I support that addition but do not have the time to implement it right
>> now. It would also be nice to add a test case that the optimized
>> assignment path does _not_ happen for pre-allocated SBufs.
>>
> 
> Test case added as trunk rev.14911
> 
> SBuf::append patch applied as trunk rev.14912

Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Extend SBufContainerJoin to have prefix and suffix arguments

[Alex Rousskov]

On 11/04/2016 01:12 AM, Kinkie wrote:
> On Thu, Nov 3, 2016 at 10:55 PM, Alex Rousskov
>  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 (SBuf , ...);

> attached v3 does this.

>  SBuf
> +JoinContainerIntoSBuf(SBuf , ...

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 

Re: [squid-dev] [PATCH] Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.

[Christos Tsantilas]

The SQUID-211-Skype_groups_and_msnp_bypass-t11.patch applied to trunk as 
r14913

The SQUID-211-FwdState_connectStart_cleanup-t1.patch patch applied as r14914

The final applied patch has one more change over the t10 patch, it 
merges the ConnStateData::startPeekAndSplice() and 
CoinnStateData::startPeekAndSpliceDone() to one method.
This is possible because the r14898 and this patch removes any extra 
call from old startPeekAndSplice method.


I am attaching the final patch as t11.


On 11/02/2016 12:59 AM, Amos Jeffries wrote:

On 2/11/2016 4:31 a.m., Christos Tsantilas wrote:

On 10/28/2016 01:11 PM, Amos Jeffries wrote:

On 21/10/2016 3:55 a.m., Christos Tsantilas wrote:


Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.

Use case: Skype groups appear to use TLS-encrypted MSNP protocol instead
of HTTPS. This change allows Squid admins using SslBump to tunnel Skype
groups and similar non-HTTP traffic bytes via "on_unsupported_protocol
tunnel all". Previously, the combination resulted in encrypted HTTP 400
(Bad Request) messages sent to the client (that does not speak HTTP).

Also this patch:
 * fixes bug 4529: !EBIT_TEST(entry->flags, ENTRY_FWD_HDR_WAIT)
   assertion in FwdState.cc.

 * when splicing transparent connections during SslBump step1, avoid
   access-logging an extra record and log %ssl::bump_mode as the
expected
   "splice" not "none".

 * handles an XXX comment inside clientTunnelOnError for possible memory
   leak of client streams related objects

 * fixes TunnelStateData logging in the case of splicing after peek.

This is a Measurement Factory project.



Are any of these additional fixes able to be easily broken out into
separate patches? It would greatly help the auditing process to get
smaller patches.

in src/FwdState.cc:

* Most of the changed lines are a needless de-scoping of code.
 - note that the sub-scope existed to begin with so as to silence noisy
false-positives by static analysis tools (trunk rev.14790).


I make FwdState related fixes a separate patch and I remove it from main
patch.
Do you believe that the SQUID-211-FwdState_connectStart_cleanup-t1.patch
will cause problems to Squid?
We can just remove the line :
  pinned_connection->stopPinnedConnectionMonitoring();



It is not problems in Squid itself, but in the analysis tools being
confused. We can try this one and see how it goes.




 - please take advantage of the surrounding code being re-written to
cleanup:
if (peer_paths == NULL || peer_paths->size() < 1) {
debugs(26, 3, HERE << "No paths found. Aborting CONNECT");
as:
if (!peer_paths || peer_paths->size() < 1) {
debugs(26, 3, "No paths found. Aborting CONNECT");



I did not this fix Amos.
I will make a separate commit after this patch committed if you want it.



I'm okay with that. Please do it the way Alex suggested though that is
slightly better.

+1.

Amos




Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.

Use case: Skype groups appear to use TLS-encrypted MSNP protocol instead
of HTTPS. This change allows Squid admins using SslBump to tunnel Skype
groups and similar non-HTTP traffic bytes via "on_unsupported_protocol
tunnel all". Previously, the combination resulted in encrypted HTTP 400
(Bad Request) messages sent to the client (that does not speak HTTP).

Also this patch:
 * fixes bug 4529: !EBIT_TEST(entry->flags, ENTRY_FWD_HDR_WAIT)
   assertion in FwdState.cc.

 * when splicing transparent connections during SslBump step1, avoid
   access-logging an extra record and log %ssl::bump_mode as the expected
   "splice" not "none".

 * handles an XXX comment inside clientTunnelOnError for possible memory
   leak of client streams related objects

 * fixes TunnelStateData logging in the case of splicing after peek.

This is a Measurement Factory project.

=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc	2016-08-17 00:38:25 +
+++ src/HttpRequest.cc	2016-10-20 14:46:05 +
@@ -640,25 +640,25 @@
 {
 if (clientConnectionManager.valid() && clientConnectionManager->pinning.pinned)
 return clientConnectionManager.get();
 return NULL;
 }
 
 const SBuf
 HttpRequest::storeId()
 {
 if (store_id.size() != 0) {
 debugs(73, 3, "sent back store_id: " << store_id);
 return StringToSBuf(store_id);
 }
 debugs(73, 3, "sent back effectiveRequestUrl: " << effectiveRequestUri());
 return effectiveRequestUri();
 }
 
 const SBuf &
 HttpRequest::effectiveRequestUri() const
 {
-if (method.id() == Http::METHOD_CONNECT)
+if (method.id() == Http::METHOD_CONNECT || url.getScheme() == AnyP::PROTO_AUTHORITY_FORM)
 return url.authority(true); // host:port
 return url.absolute();
 }
 

=== modified file 'src/RequestFlags.h'
--- src/RequestFlags.h	2016-01-01 00:12:18 +
+++ src/RequestFlags.h	2016-10-04 07:46:28 +
@@ -99,38 +99,41 @@
 bool chunkedReply :1;
 /** set if stream error has occured */
 bool streamError :1;
 

Re: [squid-dev] [PATCH] Faster SBuf::append

[Amos Jeffries]

On 7/10/2016 6:20 a.m., Alex Rousskov wrote:
> On 10/06/2016 10:57 AM, Amos Jeffries wrote:
> 
>> Please add a check to the unit test testSBuf::testAppendSBuf()
>> to guarantee that the (*this = S) assignment code path updates the store
>> reference count rather than doing a bit-wise copy of the SBuf.
> 
> I support that addition but do not have the time to implement it right
> now. It would also be nice to add a test case that the optimized
> assignment path does _not_ happen for pre-allocated SBufs.
> 

Test case added as trunk rev.14911

SBuf::append patch applied as trunk rev.14912

Amos
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Extend SBufContainerJoin to have prefix and suffix arguments

[Kinkie]

On Thu, Nov 3, 2016 at 10:55 PM, Alex Rousskov
 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 (SBuf , ...);
>
>
>> Can simply return the modified SBuf.
>
>>  SBuf
>> -SBufContainerJoin(const Container , const SBuf& separator)
>> +JoinContainerIntoSBuf(SBuf dest, const ContainerIterator ,
>
> This implementation does not avoid copies in a general case: When I
> already have an SBuf with some content, I have to feed my writeable SBuf
> to ContainerToSBuf() to avoid allocating and copying.
>
> AFAICT, the reasonable implementation options are:
>
>   1. Your old simpler implementation without "dest".
>   2. A more efficient implementation with writeable "dest".

attached v3 does this.

> The latest implementation has the complexity of #2 but lacks its
> efficiency. I do not think it is a good API.

I am not generally worried as it seems you are about copying SBufs as
long as the underlying storage is not reallocated needlessly. But it's
very simple to express one call in terms of the other, so I'm doing
just that. I am convinced that the end result of our discussion is
better than what we had started with, and I'm grateful for that.


-- 
Francesco


sbufcontainerjoin-v3.patch
Description: Binary data
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev