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 c08d01a53897e0fe7020eceed793d25497e941b2..9291036afb95c2a37d3659da154f2d124a1a173b 100644 (file)
--- 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 8681d1aaca020564d88cb977ca5d44a9c2d693e4..be6d43c7b0a2b6deef83610da81ad8d34591b411 100644 (file)
--- 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 3b8ca169b96971c1a65b1982ea5da3a912ff6b7e..44505bcf64d7ee46a2f78137ea94e41af5f98e43 100644 (file)
--- 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 6f30581921bf8058c1b8392426fd0e00933c52f7..49743e7663c215e172eca0e16a3a62377cfc3462 100644 (file)
--- 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 dc18b3351517c0a4db0d3b70ca570bde2a17d1a1..e38e69f9f01a32b905f69991cee66351baab6b3c 100644 (file)
--- 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 42885c2a9d331f6d93166cfacb348a43e8c20f7b..c2a7f0392c9e969c84c35680ed403e406ff3f002 100644 (file)
--- 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 5b6ab45b6badcf1214cf041190136fce3a0e5d18..d5f7f501e39e9a4597b382fc0ee73da9e72f7897 100644 (file)
--- 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 4ad6678bac0a114ef3ef3175742ffe3429fb9aba..b48e093e4672231283eda5b9030d516ada0c666c 100644 (file)
--- 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 : "<NULL>") << "}");
 
-    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));