Re: squid3 comments

On Wed, 2007-02-28 at 20:48 +0200, Tsantilas Christos wrote:

> As an example of such changes I am attaching the rewritten
> parseHttpRequest, prepareTransparentURL and prepareAcceleratedURL
>
> A second example: again In parseHttpRequest we have the HttpParser
> struct which we are using it to parse the first line of request. The
> HttpRequest::parseFirstLine method (of HttpMsg derived HttpRequest
> class) does more or less the same. Maybe they can merged.
>
> A third example: in HttpStateData::processReplyHeader int http.cc file.
> I am seeing the line:
> const bool parsed = newrep->parse(readBuf, eof, &error);
>
> and some lines after I am seeing:
> header_bytes_read = headersEnd(readBuf->content(),
> readBuf->contentSize());
>
> What the hell parsing we did in previous line if we did not keep the end
> of headers? The easier we can do is to make HttpReply::parse to return
> the size of headers on success or zero else. Or just keep in an
> internall variable of HttpReply class the size of headers and then get
> them with a call like newrep->headersSize()
>
> I think such changes are small and can make squid3 to be a little faster
> and can simplify in some cases the code, which will help the debuging.
> I am not having a lot of time too but I can find some hours here or
> there to make such changes, if needed.

If you are absolutely, positively, 100% sure that these changes are
correct then it is only a question whether they will help us make Squid3
stable faster (by fixing bugs, improving the code, debugging flow, or
whatever). If they will, we should implement them.

My concern is that in many cases, an innocent-looking parsing function
has a couple of well-hidden side effects. Removing a function call or
changing calls order introduces subtle bugs. I have been bitten by this
many times.

I think we should start from stating the goal of these changes in Squid
3.0. If they are for performance improvement, I would suggest waiting
until v3.1 or until performance tests indicate that we must improve
Squid 3 performance before calling it stable. If the goal is to fix a
common-case bug, we should make the necessary changes, of course. If the
ultimate goal is different, let's discuss it.

Thank you,

Alex.
P.S. I will try to look at your specific changes soon.
Received on Wed Feb 28 2007 - 17:54:14 MST