Hello,
Attached is a patch with the suggested changes by Amos.
Amos, thank you for taking the time to look at this.
Thank you,
Nathan.
On 19 March 2016 at 19:34, Amos Jeffries <[email protected]> wrote:
> On 15/03/2016 12:33 p.m., Nathan Hoad wrote:
>> 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.
>
>
> in format/Format.cc:
>
> * avoid c_str() like below. Which still copies, but avoids also
> reallocating the ALE url field:
>
> if (!al->url.isEmpty()) {
> const SBuf &s = al->url;
> sb.append(s.rawContent(), s.length());
> out = sb.termedBuf();
> }
>
>
> in src/log/FormatHttpdCombined.cc:
>
> * in printf()-like statements we have the SQUIDSBUFPH and
> SQUIDSBUFPRINT(x) macros which output an SBuf efficiently as can be done.
> - same for all the other printf-like calls.
>
>
> in src/log/access_log.cc:
>
> * Its worth adding a global "SBuf Dash" to the Format namespace and
> using Format::Dash instead of dash_str to set the new SBuf member.
>
>
> in src/log/FormatSquidReferer.cc
>
> * use a "const SBuf s = ..." local variable which has the if-logic
> applied to it separately to assign the Format::Dash then the local is
> passed to the printf() call.
>
> Amos
>
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-21 04:49:45 +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-21 04:49:45 +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-21 04:49:45 +0000
@@ -1039,7 +1039,11 @@
break;
case LFT_REQUEST_URI:
- out = al->url;
+ if (!al->url.isEmpty()) {
+ const SBuf &s = al->url;
+ sb.append(s.rawContent(), s.length());
+ out = sb.termedBuf();
+ }
break;
case LFT_REQUEST_VERSION_OLD_2X:
=== modified file 'src/format/Format.h'
--- src/format/Format.h 2016-01-01 00:12:18 +0000
+++ src/format/Format.h 2016-03-21 04:49:45 +0000
@@ -11,6 +11,7 @@
#include "base/RefCount.h"
#include "ConfigParser.h"
+#include "SBuf.h"
/*
* Squid configuration allows users to define custom formats in
@@ -31,6 +32,7 @@
namespace Format
{
+ const SBuf Dash("-");
class Token;
=== modified file 'src/log/FormatHttpdCombined.cc'
--- src/log/FormatHttpdCombined.cc 2016-01-01 00:12:18 +0000
+++ src/log/FormatHttpdCombined.cc 2016-03-21 04:49:45 +0000
@@ -47,13 +47,13 @@
const SBuf method(al->getLogMethod());
- logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " %s %s/%d.%d\" %d %" PRId64 " \"%s\" \"%s\" %s:%s%s",
+ logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\" %d %" PRId64 " \"%s\" \"%s\" %s:%s%s",
clientip,
user_ident ? user_ident : dash_str,
user_auth ? user_auth : dash_str,
Time::FormatHttpd(squid_curtime),
SQUIDSBUFPRINT(method),
- al->url,
+ SQUIDSBUFPRINT(al->url),
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-21 04:49:45 +0000
@@ -34,13 +34,13 @@
const SBuf method(al->getLogMethod());
- logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " %s %s/%d.%d\" %d %" PRId64 " %s:%s%s",
+ logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\" %d %" PRId64 " %s:%s%s",
clientip,
user_ident ? user_ident : dash_str,
user_auth ? user_auth : dash_str,
Time::FormatHttpd(squid_curtime),
SQUIDSBUFPRINT(method),
- al->url,
+ SQUIDSBUFPRINT(al->url),
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-21 04:49:45 +0000
@@ -50,7 +50,7 @@
const SBuf method(al->getLogMethod());
- logfilePrintf(logfile, "%9ld.%03d %6ld %s %s/%03d %" PRId64 " " SQUIDSBUFPH " %s %s %s%s/%s %s%s",
+ logfilePrintf(logfile, "%9ld.%03d %6ld %s %s/%03d %" PRId64 " " SQUIDSBUFPH " " SQUIDSBUFPH " %s %s%s/%s %s%s",
(long int) current_time.tv_sec,
(int) current_time.tv_usec / 1000,
tvToMsec(al->cache.trTime),
@@ -59,7 +59,7 @@
al->http.code,
al->http.clientReplySz.messageTotal(),
SQUIDSBUFPRINT(method),
- al->url,
+ SQUIDSBUFPRINT(al->url),
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-21 04:49:45 +0000
@@ -28,11 +28,13 @@
char clientip[MAX_IPSTRLEN];
al->getLogClientIp(clientip, MAX_IPSTRLEN);
- logfilePrintf(logfile, "%9ld.%03d %s %s %s\n",
+ const SBuf url = !al->url.isEmpty() ? al->url : ::Format::Dash;
+
+ logfilePrintf(logfile, "%9ld.%03d %s %s " SQUIDSTRINGPH "\n",
(long int) current_time.tv_sec,
(int) current_time.tv_usec / 1000,
clientip,
referer,
- al->url ? al->url : "-");
+ SQUIDSBUFPRINT(url));
}
=== 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-21 04:49:45 +0000
@@ -76,8 +76,8 @@
accessLogLogTo(CustomLog* log, AccessLogEntry::Pointer &al, ACLChecklist * checklist)
{
- if (al->url == NULL)
- al->url = dash_str;
+ if (al->url.isEmpty())
+ al->url = Format::Dash;
if (!al->http.content_type || *al->http.content_type == '\0')
al->http.content_type = dash_str;
@@ -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
