From: Amos Jeffries Date: Mon, 6 Jan 2014 15:54:56 +0000 (-0800) Subject: Performance: pass size details to HttpHeader parse logic X-Git-Tag: merge-candidate-3-v1~506^2~57 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=784619e657b974bc50b804d3b9e5161790490369;p=thirdparty%2Fsquid.git Performance: pass size details to HttpHeader parse logic The Http1::RequestParser class is already aware of the location and size of the mime-header block so there is no need to scan for it again. This removes the need for httpMsgIsolateHeaders() outside of HttpMsg and the 1-2 data scans it performs over the mime-header block per HTTP request. Also, supporting changes required in HTCP to use the new HttpHeader::parseHeaders API removes 10x strlen() calls over the HTCP mime-headers payload and similar large memory blocks per HTCP packet. --- diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc index c08d01a538..9291036afb 100644 --- a/src/HttpHeader.cc +++ b/src/HttpHeader.cc @@ -545,9 +545,10 @@ HttpHeader::reset() } int -HttpHeader::parse(const char *header_start, const char *header_end) +HttpHeader::parse(const char *header_start, size_t hdrLen) { const char *field_ptr = header_start; + const char *header_end = header_start + hdrLen; // XXX: remove HttpHeaderEntry *e, *e2; bool warnOnError = (Config.onoff.relaxed_header_parser <= 0 ? DBG_IMPORTANT : 2); @@ -558,7 +559,7 @@ HttpHeader::parse(const char *header_start, const char *header_end) ++ HttpHeaderStats[owner].parsedCount; char *nulpos; - if ((nulpos = (char*)memchr(header_start, '\0', header_end - header_start))) { + if ((nulpos = (char*)memchr(header_start, '\0', hdrLen))) { debugs(55, DBG_IMPORTANT, "WARNING: HTTP header contains NULL characters {" << getStringPrefix(header_start, nulpos) << "}\nNULL\n{" << getStringPrefix(nulpos+1, header_end)); PROF_stop(HttpHeaderParse); diff --git a/src/HttpHeader.h b/src/HttpHeader.h index 8681d1aaca..be6d43c7b0 100644 --- a/src/HttpHeader.h +++ b/src/HttpHeader.h @@ -234,7 +234,7 @@ public: void update (HttpHeader const *fresh, HttpHeaderMask const *denied_mask); void compact(); int reset(); - int parse(const char *header_start, const char *header_end); + int parse(const char *header_start, size_t len); void packInto(Packer * p, bool mask_sensitive_info=false) const; HttpHeaderEntry *getEntry(HttpHeaderPos * pos) const; HttpHeaderEntry *findEntry(http_hdr_type id) const; diff --git a/src/HttpMsg.cc b/src/HttpMsg.cc index 3b8ca169b9..44505bcf64 100644 --- a/src/HttpMsg.cc +++ b/src/HttpMsg.cc @@ -58,7 +58,7 @@ HttpMsgParseState &operator++ (HttpMsgParseState &aState) } /* find end of headers */ -int +static int httpMsgIsolateHeaders(const char **parse_start, int l, const char **blk_start, const char **blk_end) { /* @@ -278,14 +278,15 @@ HttpMsg::httpMsgParseStep(const char *buf, int len, int atEnd) if (pstate == psReadyToParseHeaders) { if (!httpMsgIsolateHeaders(&parse_start, parse_len, &blk_start, &blk_end)) { if (atEnd) { - blk_start = parse_start, blk_end = blk_start + strlen(blk_start); + blk_start = parse_start; + blk_end = blk_start + strlen(blk_start); } else { PROF_stop(HttpMsg_httpMsgParseStep); return 0; } } - if (!header.parse(blk_start, blk_end)) { + if (!header.parse(blk_start, blk_end-blk_start)) { PROF_stop(HttpMsg_httpMsgParseStep); return httpMsgParseError(); } diff --git a/src/HttpMsg.h b/src/HttpMsg.h index 6f30581921..49743e7663 100644 --- a/src/HttpMsg.h +++ b/src/HttpMsg.h @@ -122,8 +122,6 @@ protected: virtual void hdrCacheInit(); }; -int httpMsgIsolateHeaders(const char **parse_start, int len, const char **blk_start, const char **blk_end); - #define HTTPMSGUNLOCK(a) if (a) { if ((a)->unlock() == 0) delete (a); (a)=NULL; } #define HTTPMSGLOCK(a) (a)->lock() diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index dc18b33515..e38e69f9f0 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -41,6 +41,7 @@ #include "globals.h" #include "gopher.h" #include "http.h" +#include "http/Http1Parser.h" #include "HttpHdrCc.h" #include "HttpHeaderRange.h" #include "HttpRequest.h" @@ -358,15 +359,13 @@ HttpRequest::parseFirstLine(const char *start, const char *end) return true; } -int -HttpRequest::parseHeader(const char *parse_start, int len) +bool +HttpRequest::parseHeader(Http1::RequestParser &hp) { - const char *blk_start, *blk_end; - - if (!httpMsgIsolateHeaders(&parse_start, len, &blk_start, &blk_end)) - return 0; + if (!hp.headerBlockSize()) + return false; - int result = header.parse(blk_start, blk_end); + bool result = header.parse(hp.rawHeaderBuf(), hp.headerBlockSize()); if (result) hdrCacheInit(); diff --git a/src/HttpRequest.h b/src/HttpRequest.h index 42885c2a9d..c2a7f0392c 100644 --- a/src/HttpRequest.h +++ b/src/HttpRequest.h @@ -227,7 +227,7 @@ public: bool parseFirstLine(const char *start, const char *end); - int parseHeader(const char *parse_start, int len); + bool parseHeader(Http1::RequestParser &hp); // TODO move this function to the parser virtual bool expectingBody(const HttpRequestMethod& unused, int64_t&) const; diff --git a/src/client_side.cc b/src/client_side.cc index 5b6ab45b6b..d5f7f501e3 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2601,7 +2601,7 @@ clientProcessRequest(ConnStateData *conn, Http1::RequestParser &hp, ClientSocket } /* compile headers */ - if (http_ver.major >= 1 && !request->parseHeader(hp.rawHeaderBuf(), hp.headerBlockSize())) { + if (http_ver.major >= 1 && !request->parseHeader(hp)) { clientStreamNode *node = context->getClientReplyContext(); debugs(33, 5, "Failed to parse request headers:\n" << hp.rawHeaderBuf()); conn->quitAfterError(request.getRaw()); diff --git a/src/htcp.cc b/src/htcp.cc index 4ad6678bac..b48e093e46 100644 --- a/src/htcp.cc +++ b/src/htcp.cc @@ -172,6 +172,7 @@ public: char *uri; char *version; char *req_hdrs; + size_t reqHdrsSz; ///< size of the req_hdrs content HttpRequest *request; private: @@ -185,8 +186,13 @@ MEMPROXY_CLASS_INLINE(htcpSpecifier); struct _htcpDetail { char *resp_hdrs; + size_t respHdrsSz; + char *entity_hdrs; + size_t entityHdrsSz; + char *cache_hdrs; + size_t cacheHdrsSz; }; struct _htcpStuff { @@ -256,7 +262,7 @@ static ssize_t htcpBuildPacket(char *buf, size_t buflen, htcpStuff * stuff); static htcpSpecifier *htcpUnpackSpecifier(char *buf, int sz); static htcpDetail *htcpUnpackDetail(char *buf, int sz); static ssize_t htcpBuildAuth(char *buf, size_t buflen); -static ssize_t htcpBuildCountstr(char *buf, size_t buflen, const char *s); +static ssize_t htcpBuildCountstr(char *buf, size_t buflen, const char *s, size_t len); static ssize_t htcpBuildData(char *buf, size_t buflen, htcpStuff * stuff); static ssize_t htcpBuildDetail(char *buf, size_t buflen, htcpStuff * stuff); static ssize_t htcpBuildOpData(char *buf, size_t buflen, htcpStuff * stuff); @@ -330,25 +336,18 @@ htcpBuildAuth(char *buf, size_t buflen) } static ssize_t -htcpBuildCountstr(char *buf, size_t buflen, const char *s) +htcpBuildCountstr(char *buf, size_t buflen, const char *s, size_t len) { - uint16_t length; - size_t len; int off = 0; if (buflen - off < 2) return -1; - if (s) - len = strlen(s); - else - len = 0; - debugs(31, 3, "htcpBuildCountstr: LENGTH = " << len); debugs(31, 3, "htcpBuildCountstr: TEXT = {" << (s ? s : "") << "}"); - length = htons((uint16_t) len); + uint16_t length = htons((uint16_t) len); memcpy(buf + off, &length, 2); @@ -370,28 +369,28 @@ htcpBuildSpecifier(char *buf, size_t buflen, htcpStuff * stuff) { ssize_t off = 0; ssize_t s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.method); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.method, (stuff->S.method?strlen(stuff->S.method):0)); if (s < 0) return s; off += s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.uri); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.uri, (stuff->S.uri?strlen(stuff->S.uri):0)); if (s < 0) return s; off += s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.version); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.version, (stuff->S.version?strlen(stuff->S.version):0)); if (s < 0) return s; off += s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.req_hdrs); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->S.req_hdrs, stuff->S.reqHdrsSz); if (s < 0) return s; @@ -408,21 +407,21 @@ htcpBuildDetail(char *buf, size_t buflen, htcpStuff * stuff) { ssize_t off = 0; ssize_t s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->D.resp_hdrs); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->D.resp_hdrs, stuff->D.respHdrsSz); if (s < 0) return s; off += s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->D.entity_hdrs); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->D.entity_hdrs, stuff->D.entityHdrsSz); if (s < 0) return s; off += s; - s = htcpBuildCountstr(buf + off, buflen - off, stuff->D.cache_hdrs); + s = htcpBuildCountstr(buf + off, buflen - off, stuff->D.cache_hdrs, stuff->D.cacheHdrsSz); if (s < 0) return s; @@ -651,6 +650,8 @@ htcpFreeDetail(htcpDetail * d) * Unpack an HTCP SPECIFIER in place * This will overwrite any following AUTH block */ +// XXX: this needs to be turned into an Htcp1::Parser inheriting from Http1::RequestParser +// but with different first-line and block unpacking logic. static htcpSpecifier * htcpUnpackSpecifier(char *buf, int sz) { @@ -732,6 +733,7 @@ htcpUnpackSpecifier(char *buf, int sz) s->req_hdrs = buf; buf += l; sz -= l; + s->reqHdrsSz = l; debugs(31, 6, "htcpUnpackSpecifier: REQ-HDRS (" << l << "/" << sz << ") '" << s->req_hdrs << "'"); debugs(31, 3, "htcpUnpackSpecifier: " << sz << " bytes left"); @@ -778,9 +780,8 @@ htcpUnpackDetail(char *buf, int sz) /* Set RESP-HDRS */ d->resp_hdrs = buf; - buf += l; - + d->respHdrsSz = l; sz -= l; /* Find length of ENTITY-HDRS */ @@ -801,9 +802,8 @@ htcpUnpackDetail(char *buf, int sz) buf += 2; d->entity_hdrs = buf; - buf += l; - + d->entityHdrsSz = l; sz -= l; /* Find length of CACHE-HDRS */ @@ -824,9 +824,8 @@ htcpUnpackDetail(char *buf, int sz) buf += 2; d->cache_hdrs = buf; - buf += l; - + d->cacheHdrsSz = l; sz -= l; debugs(31, 3, "htcpUnpackDetail: " << sz << " bytes left"); @@ -878,12 +877,14 @@ htcpTstReply(htcpDataHeader * dhdr, StoreEntry * e, htcpSpecifier * spec, Ip::Ad stuff.S.uri = spec->uri; stuff.S.version = spec->version; stuff.S.req_hdrs = spec->req_hdrs; + stuff.S.reqHdrsSz = spec->reqHdrsSz; if (e) hdr.putInt(HDR_AGE, (e->timestamp <= squid_curtime ? (squid_curtime - e->timestamp) : 0) ); else hdr.putInt(HDR_AGE, 0); hdr.packInto(&p); stuff.D.resp_hdrs = xstrdup(mb.buf); + stuff.D.respHdrsSz = mb.contentSize(); debugs(31, 3, "htcpTstReply: resp_hdrs = {" << stuff.D.resp_hdrs << "}"); mb.reset(); hdr.reset(); @@ -897,6 +898,7 @@ htcpTstReply(htcpDataHeader * dhdr, StoreEntry * e, htcpSpecifier * spec, Ip::Ad hdr.packInto(&p); stuff.D.entity_hdrs = xstrdup(mb.buf); + stuff.D.entityHdrsSz = mb.contentSize(); debugs(31, 3, "htcpTstReply: entity_hdrs = {" << stuff.D.entity_hdrs << "}"); @@ -922,6 +924,7 @@ htcpTstReply(htcpDataHeader * dhdr, StoreEntry * e, htcpSpecifier * spec, Ip::Ad hdr.packInto(&p); stuff.D.cache_hdrs = xstrdup(mb.buf); + stuff.D.cacheHdrsSz = mb.contentSize(); debugs(31, 3, "htcpTstReply: cache_hdrs = {" << stuff.D.cache_hdrs << "}"); mb.clean(); hdr.clean(); @@ -931,8 +934,11 @@ htcpTstReply(htcpDataHeader * dhdr, StoreEntry * e, htcpSpecifier * spec, Ip::Ad pktlen = htcpBuildPacket(pkt, sizeof(pkt), &stuff); safe_free(stuff.D.resp_hdrs); + stuff.D.respHdrsSz = 0; safe_free(stuff.D.entity_hdrs); + stuff.D.entityHdrsSz = 0; safe_free(stuff.D.cache_hdrs); + stuff.D.cacheHdrsSz = 0; if (!pktlen) { debugs(31, 3, "htcpTstReply: htcpBuildPacket() failed"); @@ -989,7 +995,6 @@ htcpHandleNop(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from) void htcpSpecifier::checkHit() { - char *blk_end; checkHitRequest = request; if (NULL == checkHitRequest) { @@ -998,9 +1003,7 @@ htcpSpecifier::checkHit() return; } - blk_end = req_hdrs + strlen(req_hdrs); - - if (!checkHitRequest->header.parse(req_hdrs, blk_end)) { + if (!checkHitRequest->header.parse(req_hdrs, reqHdrsSz)) { debugs(31, 3, "htcpCheckHit: NO; failed to parse request headers"); delete checkHitRequest; checkHitRequest = NULL; @@ -1042,7 +1045,6 @@ static int htcpClrStore(const htcpSpecifier * s) { HttpRequest *request = s->request; - char *blk_end; StoreEntry *e = NULL; int released = 0; @@ -1052,9 +1054,7 @@ htcpClrStore(const htcpSpecifier * s) } /* Parse request headers */ - blk_end = s->req_hdrs + strlen(s->req_hdrs); - - if (!request->header.parse(s->req_hdrs, blk_end)) { + if (!request->header.parse(s->req_hdrs, s->reqHdrsSz)) { debugs(31, 2, "htcpClrStore: failed to parse request headers"); return -1; } @@ -1147,13 +1147,13 @@ htcpHandleTstResponse(htcpDataHeader * hdr, char *buf, int sz, Ip::Address &from } if ((t = d->resp_hdrs)) - htcpReply.hdr.parse(t, t + strlen(t)); + htcpReply.hdr.parse(t, d->respHdrsSz); if ((t = d->entity_hdrs)) - htcpReply.hdr.parse(t, t + strlen(t)); + htcpReply.hdr.parse(t, d->entityHdrsSz); if ((t = d->cache_hdrs)) - htcpReply.hdr.parse(t, t + strlen(t)); + htcpReply.hdr.parse(t, d->cacheHdrsSz); } debugs(31, 3, "htcpHandleTstResponse: key (" << key << ") " << storeKeyText(key));