Hello,
Attached is a first attempt at converting the AccessLogEntry::url
member to an SBuf. It's definitely resulted in more data copies - just
about all sources are still char *.
I'm not sure how I can actually avoid those, aside from converting
them to SBuf as well, which would mean modifying HTCP and ICP logging,
and ClientSideRequest::log_uri. I can take a crack at
ClientSideRequest::log_uri if people are interested, but I'm not in a
position to test any changes made to HTCP and ICP logging.
Thank you,
Nathan.
On 11 March 2016 at 16:21, Amos Jeffries <[email protected]> wrote:
> On 11/03/2016 12:53 p.m., Nathan Hoad wrote:
>> Hello,
>>
>> The attached patch is a demonstration of a memory leak on
>> AccessLogentry::url, created by ACLFilledChecklist::syncAle(). The
>> patch itself is not good enough quality to warrant acceptance, and no
>> doubt introduces bugs.
>>
>> ::syncAle() is the only place in the codebase that assigns a URL that
>> AccessLogEntry is expected to free(), which AccessLogEntry doesn't do.
>> This results in a memory leak.
>>
>> As mentioned, the patch is of questionable quality, as the chunk in
>> src/client_side.cc shows. This sort of change would have to be made
>> everywhere that url is assigned to, which isn't practical.
>>
>> It would be good to modify ::syncAle() so that it doesn't allocate a
>> new URL, but I'm not sure how that could be achieved. I think the only
>> workable alternative is to change AccessLogEntry::url to a more
>> intelligent string type. There is also the possibility of making the
>> member private and using a setter to ensure it is always xstrdup'd
>> from another URL.
>>
>> If someone with better knowledge than me would like to make a
>> recommendation on what to do here, I'd be happy to work towards a
>> better solution.
>
> You can probably guess by now that I'm going to say SBuf.
>
> In this case there is also possibly the option of removing that url
> variable entirely. A class URL is the proper way to store URLs. But that
> could be significantly more work to make URL ref-countable with a Pointer.
>
> FYI:
> SBuf is the intended solution to all our many string related problems.
> We are just in the transitional period of converting things.
>
> One issue we are working around during the transition time is that SBuf
> data-copies when importing from any non-SBuf type, and when c_str() is
> called to export. Effort to avoid those is worthwhile. Use in the wrong
> places can reduce performance noticably.
> However, any object which is already being allocated or copied to the
> string/char* can be SBuf converted without this being a new cost. And we
> then gain improved memory management from it.
>
> Amos
>
> _______________________________________________
> squid-dev mailing list
> [email protected]
> http://lists.squid-cache.org/listinfo/squid-dev
Convert AccessLogEntry::url to an SBuf. This eliminates at least one memory leak.
This work is submitted on behalf of Bloomberg L.P.
=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h 2016-03-11 15:03:20 +0000
+++ src/AccessLogEntry.h 2016-03-14 04:13:24 +0000
@@ -21,6 +21,7 @@
#include "LogTags.h"
#include "MessageSizes.h"
#include "Notes.h"
+#include "SBuf.h"
#if ICAP_CLIENT
#include "adaptation/icap/Elements.h"
#endif
@@ -56,7 +57,7 @@
/// Fetch the transaction method string (ICP opcode, HTCP opcode or HTTP method)
SBuf getLogMethod() const;
- const char *url;
+ SBuf url;
/// TCP/IP level details about the client connection
Comm::ConnectionPointer tcpClient;
=== modified file 'src/acl/FilledChecklist.cc'
--- src/acl/FilledChecklist.cc 2016-01-31 12:05:30 +0000
+++ src/acl/FilledChecklist.cc 2016-03-14 04:16:21 +0000
@@ -103,9 +103,9 @@
HTTPMSGLOCK(al->adapted_request);
}
- if (!al->url) {
+ if (al->url.isEmpty()) {
showDebugWarning("URL");
- al->url = xstrdup(request->url.absolute().c_str());
+ al->url = request->url.absolute();
}
}
=== modified file 'src/format/Format.cc'
--- src/format/Format.cc 2016-03-11 15:03:20 +0000
+++ src/format/Format.cc 2016-03-14 04:16:21 +0000
@@ -1039,7 +1039,9 @@
break;
case LFT_REQUEST_URI:
- out = al->url;
+ if (!al->url.isEmpty()) {
+ out = al->url.c_str();
+ }
break;
case LFT_REQUEST_VERSION_OLD_2X:
=== modified file 'src/log/FormatHttpdCombined.cc'
--- src/log/FormatHttpdCombined.cc 2016-01-01 00:12:18 +0000
+++ src/log/FormatHttpdCombined.cc 2016-03-14 04:16:21 +0000
@@ -53,7 +53,7 @@
user_auth ? user_auth : dash_str,
Time::FormatHttpd(squid_curtime),
SQUIDSBUFPRINT(method),
- al->url,
+ al->url.c_str(),
AnyP::ProtocolType_str[al->http.version.protocol],
al->http.version.major, al->http.version.minor,
al->http.code,
=== modified file 'src/log/FormatHttpdCommon.cc'
--- src/log/FormatHttpdCommon.cc 2016-01-01 00:12:18 +0000
+++ src/log/FormatHttpdCommon.cc 2016-03-14 04:16:22 +0000
@@ -40,7 +40,7 @@
user_auth ? user_auth : dash_str,
Time::FormatHttpd(squid_curtime),
SQUIDSBUFPRINT(method),
- al->url,
+ al->url.c_str(),
AnyP::ProtocolType_str[al->http.version.protocol],
al->http.version.major, al->http.version.minor,
al->http.code,
=== modified file 'src/log/FormatSquidNative.cc'
--- src/log/FormatSquidNative.cc 2016-01-01 00:12:18 +0000
+++ src/log/FormatSquidNative.cc 2016-03-14 04:16:22 +0000
@@ -59,7 +59,7 @@
al->http.code,
al->http.clientReplySz.messageTotal(),
SQUIDSBUFPRINT(method),
- al->url,
+ al->url.c_str(),
user ? user : dash_str,
al->hier.ping.timedout ? "TIMEOUT_" : "",
hier_code_str[al->hier.code],
=== modified file 'src/log/FormatSquidReferer.cc'
--- src/log/FormatSquidReferer.cc 2016-01-01 00:12:18 +0000
+++ src/log/FormatSquidReferer.cc 2016-03-14 04:16:22 +0000
@@ -33,6 +33,6 @@
(int) current_time.tv_usec / 1000,
clientip,
referer,
- al->url ? al->url : "-");
+ !al->url.isEmpty() ? al->url.c_str() : "-");
}
=== modified file 'src/log/access_log.cc'
--- src/log/access_log.cc 2016-01-01 00:12:18 +0000
+++ src/log/access_log.cc 2016-03-14 04:16:10 +0000
@@ -76,7 +76,7 @@
accessLogLogTo(CustomLog* log, AccessLogEntry::Pointer &al, ACLChecklist * checklist)
{
- if (al->url == NULL)
+ if (al->url.isEmpty())
al->url = dash_str;
if (!al->http.content_type || *al->http.content_type == '\0')
@@ -160,8 +160,8 @@
else {
unsigned int ibuf[365];
size_t isize;
- xstrncpy((char *) ibuf, al->url, 364 * sizeof(int));
- isize = ((strlen(al->url) + 8) / 8) * 2;
+ xstrncpy((char *) ibuf, al->url.c_str(), 364 * sizeof(int));
+ isize = ((al->url.length() + 8) / 8) * 2;
if (isize > 364)
isize = 364;
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
