From 4959e21ebb1baaa931f655d5be811d053f2f3bc3 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sat, 26 Mar 2011 15:03:49 +1300 Subject: [PATCH] Cleanup: make clientParseRequest() a member of ConnStateData This allows the HttpParser to also become a member field and persistent across all requests on the connection instead of newely allocated on the stack for every read cycle. That in turn allows the parser to retain state for efficient 'trickle' parsing across multiple read cycles. For now the old behaviour of reset on every read is retained in order to prevent this shuffling from causing behaviour changes. That negates most of the actual performance gains (for now). --- src/HttpMsg.cc | 5 +++++ src/HttpMsg.h | 3 +++ src/client_side.cc | 50 +++++++++++++++++++--------------------------- src/client_side.h | 5 +++++ 4 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/HttpMsg.cc b/src/HttpMsg.cc index 9c9f5c6a90..7f1db0176f 100644 --- a/src/HttpMsg.cc +++ b/src/HttpMsg.cc @@ -434,6 +434,11 @@ HttpParserRequestLen(HttpParser *hp) } #endif +HttpParser::HttpParser(const char *buf, int len) +{ + HttpParserInit(this, buf, len); +} + int HttpParser::parseRequestFirstLine() { diff --git a/src/HttpMsg.h b/src/HttpMsg.h index 736ec4fb3d..72a932473c 100644 --- a/src/HttpMsg.h +++ b/src/HttpMsg.h @@ -136,6 +136,9 @@ protected: class HttpParser { public: + HttpParser() {}; + HttpParser(const char *buf, int len); + /** * Attempt to parse the first line of a new request message. * diff --git a/src/client_side.cc b/src/client_side.cc index 6de2b4329b..94d61be179 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -192,7 +192,6 @@ static ClientSocketContext *ClientSocketContextNew(ClientHttpRequest *); /* other */ static IOCB clientWriteComplete; static IOCB clientWriteBodyComplete; -static bool clientParseRequest(ConnStateData * conn, bool &do_next_read); static PF clientLifetimeTimeout; static ClientSocketContext *parseHttpRequestAbort(ConnStateData * conn, const char *uri); static ClientSocketContext *parseHttpRequest(ConnStateData *, HttpParser *, HttpRequestMethod *, HttpVersion *); @@ -1532,7 +1531,7 @@ ClientSocketContext::keepaliveNextRequest() * from our read buffer we may never re-register for another client read. */ - if (clientParseRequest(conn, do_next_read)) { + if (conn->clientParseRequest(do_next_read)) { debugs(33, 3, "clientSocketContext::keepaliveNextRequest: FD " << conn->fd << ": parsed next request from buffer"); } @@ -2695,76 +2694,67 @@ connOkToAddRequest(ConnStateData * conn) * do_next_read is updated to indicate whether a read should be * scheduled. */ -static bool -clientParseRequest(ConnStateData * conn, bool &do_next_read) +bool +ConnStateData::clientParseRequest(bool &do_next_read) { HttpRequestMethod method; - ClientSocketContext *context; bool parsed_req = false; HttpVersion http_ver; - HttpParser hp; - debugs(33, 5, "clientParseRequest: FD " << conn->fd << ": attempting to parse"); + debugs(33, 5, HERE << "FD " << fd << ": attempting to parse"); // Loop while we have read bytes that are not needed for producing the body // On errors, bodyPipe may become nil, but readMoreRequests will be cleared - while (conn->in.notYetUsed > 0 && !conn->bodyPipe && - conn->flags.readMoreRequests) { - connStripBufferWhitespace (conn); + while (in.notYetUsed > 0 && !bodyPipe && flags.readMoreRequests) { + connStripBufferWhitespace(this); /* Don't try to parse if the buffer is empty */ - - if (conn->in.notYetUsed == 0) + if (in.notYetUsed == 0) break; /* Limit the number of concurrent requests to 2 */ - - if (!connOkToAddRequest(conn)) { + if (!connOkToAddRequest(this)) { break; } /* Should not be needed anymore */ /* Terminate the string */ - conn->in.buf[conn->in.notYetUsed] = '\0'; + in.buf[in.notYetUsed] = '\0'; /* Begin the parsing */ - HttpParserInit(&hp, conn->in.buf, conn->in.notYetUsed); - - /* Process request */ PROF_start(parseHttpRequest); + HttpParserInit(&parser_, in.buf, in.notYetUsed); - context = parseHttpRequest(conn, &hp, &method, &http_ver); - + /* Process request */ + ClientSocketContext *context = parseHttpRequest(this, &parser_, &method, &http_ver); PROF_stop(parseHttpRequest); /* partial or incomplete request */ if (!context) { // TODO: why parseHttpRequest can just return parseHttpRequestAbort // (which becomes context) but checkHeaderLimits cannot? - conn->checkHeaderLimits(); + checkHeaderLimits(); break; } /* status -1 or 1 */ if (context) { - debugs(33, 5, "clientParseRequest: FD " << conn->fd << ": parsed a request"); - commSetTimeout(conn->fd, Config.Timeout.lifetime, clientLifetimeTimeout, + debugs(33, 5, HERE << "FD " << fd << ": parsed a request"); + commSetTimeout(fd, Config.Timeout.lifetime, clientLifetimeTimeout, context->http); - clientProcessRequest(conn, &hp, context, method, http_ver); + clientProcessRequest(this, &parser_, context, method, http_ver); - parsed_req = true; + parsed_req = true; // XXX: do we really need to parse everything right NOW ? if (context->mayUseConnection()) { - debugs(33, 3, "clientParseRequest: Not reading, as this request may need the connection"); - do_next_read = 0; - break; + debugs(33, 3, HERE << "Not reading, as this request may need the connection"); + return false; } } } /* XXX where to 'finish' the parsing pass? */ - return parsed_req; } @@ -2835,7 +2825,7 @@ ConnStateData::clientReadRequest(const CommIoCbParams &io) if (getConcurrentRequestCount() == 0) fd_note(fd, "Reading next request"); - if (! clientParseRequest(this, do_next_read)) { + if (!clientParseRequest(do_next_read)) { if (!isOpen()) return; /* diff --git a/src/client_side.h b/src/client_side.h index 3e81238949..bc6e8d8ecf 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -151,6 +151,8 @@ public: bool areAllContextsForThisConnection() const; void freeAllContexts(); void notifyAllContexts(const int xerrno); ///< tell everybody about the err + /// Traffic parsing + bool clientParseRequest(bool &do_next_read); void readNextRequest(); bool maybeMakeSpaceAvailable(); ClientSocketContext::Pointer getCurrentContext() const; @@ -308,6 +310,9 @@ private: void clientAfterReadingRequests(int do_next_read); private: + HttpParser parser_; + + // XXX: CBDATA plays with public/private and leaves the following 'private' fields all public... :( CBDATA_CLASS2(ConnStateData); bool transparent_; bool closing_; -- 2.47.3