[squid-dev] [PREVIEW] Free AccessLogEntry::url when needed

[Nathan Hoad]

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 <squid3 at treenet.co.nz> 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
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AccessLogEntry-url-to-SBuf-v1.patch
Type: text/x-patch
Size: 4479 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160315/83e40f64/attachment-0001.bin>