Re: [squid-dev] [PATCH] Faster SBuf::append
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] Faster SBuf::append
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] Faster SBuf::append
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. >> P.S. AFAICT, there is no guarantee that SBuf using GetStorePrototype() >> isEmpty so checking both conditions seems necessary to me. > That sounds like a bug to me. If the initial prototype is anything but > empty then the resulting new/clear'ed SBuf might end up with some random > data prefix. Followup patch? Not a bug AFAICT: The initial prototype storage might get dirty, but the freshly initialized SBuf will be _empty_, making any storage bytes irrelevant. 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] Faster SBuf::append
On 7/10/2016 5:01 a.m., Alex Rousskov wrote: > Hello, > > The attached optimization patch was inspired be reviewing the > following code: > >> Parser::parse(const SBuf ) > ... >> if (preservedData_.isEmpty()) >> preservedData_ = aBuf; // avoid needless memory allocation >> else >> preservedData_.append(aBuf); > > Supporting this kind of optimization automatically was a little trickier > than I thought, but I think the attached patch does that. After the > patch, the above code will collapse to a single append call: > > preservedData_.append(aBuf); > 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. > > HTH, > > Alex. > P.S. AFAICT, there is no guarantee that SBuf using GetStorePrototype() > isEmpty so checking both conditions seems necessary to me. > That sounds like a bug to me. If the initial prototype is anything but empty then the resulting new/clear'ed SBuf might end up with some random data prefix. Followup patch? Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
[squid-dev] [PATCH] Faster SBuf::append
Hello,
The attached optimization patch was inspired be reviewing the
following code:
> Parser::parse(const SBuf )
...
> if (preservedData_.isEmpty())
> preservedData_ = aBuf; // avoid needless memory allocation
> else
> preservedData_.append(aBuf);
Supporting this kind of optimization automatically was a little trickier
than I thought, but I think the attached patch does that. After the
patch, the above code will collapse to a single append call:
preservedData_.append(aBuf);
HTH,
Alex.
P.S. AFAICT, there is no guarantee that SBuf using GetStorePrototype()
isEmpty so checking both conditions seems necessary to me.
Optimize appending to empty SBufs, a common use case.
Most appends start with an empty buffer. Some append only once. There
is no reason to allocate new memory for that first append or force the
appender to write code to optimize that first append.
Why also check that SBuf has never been configured or altered then? To
preserve pre-allocated SBufs, such as those [ab]used as I/O buffers.
=== modified file 'src/sbuf/SBuf.cc'
--- src/sbuf/SBuf.cc 2016-06-14 16:54:23 +
+++ src/sbuf/SBuf.cc 2016-10-06 15:29:26 +
@@ -170,40 +170,43 @@ SBuf::rawSpace(size_type minSpace)
void
SBuf::clear()
{
#if 0
//enabling this code path, the store will be freed and reinitialized
store_ = GetStorePrototype(); //uncomment to actually free storage upon clear()
#else
//enabling this code path, we try to release the store without deallocating it.
// will be lazily reallocated if needed.
if (store_->LockCount() == 1)
store_->clear();
#endif
len_ = 0;
off_ = 0;
++stats.clear;
}
SBuf&
SBuf::append(const SBuf )
{
+if (isEmpty() && store_ == GetStorePrototype())
+return (*this = S); // optimization: avoid needless copying
+
const Locker blobKeeper(this, S.buf());
return lowAppend(S.buf(), S.length());
}
SBuf &
SBuf::append(const char * S, size_type Ssize)
{
const Locker blobKeeper(this, S);
if (S == NULL)
return *this;
if (Ssize == SBuf::npos)
Ssize = strlen(S);
debugs(24, 7, "from c-string to id " << id);
// coverity[access_dbuff_in_call]
return lowAppend(S, Ssize);
}
SBuf &
SBuf::append(const char c)
{
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
