Re: [MERGE] c++-refactor HttpHdrCc

On 09/12/2011 05:38 PM, Kinkie wrote:
> Hi,
> the attached bundle refactors the HttpHdrCc into a c++ class,
> attempting also to clean the code up. Main changes:
> - 6 less global functions in protos.h
> - 1 less struct in structs.h
> - proper c++ construction/destruction semantics, standard MemPool integration

> - name->id header parsing now done via std::map instead of iterating
> over list: complexity should go from O(n) to O(log n) with STL-derived
> libs.

On the other hand, you now deallocate and allocate a new SquidString for
every CC directive just to find that directive ID so the total cost may
be not be lower. I think we can do much better, and even if we cannot or
will not, I do not think we should commit the "optimization" part of
this change until proper performance regression testing.

And if you want to mix optimization with cleanup, here is one place
where you can save quite a few cycles for many messages by not ignoring
the return value of getList():

> getList(HDR_CACHE_CONTROL, &s);
>
> - cc = httpHdrCcParseCreate(&s);
> + cc=new HttpHdrCc();
> + if (!cc->parseInit(s)) {
> + delete cc;
> + cc = NULL;
> + }

> +/** Http Cache-control header representation
> + *
> + * Store, parse and output the Cache-control HTTP header.

In comments, please capitalize HTTP Cache-Control the way RFC 2616 does.

> + * Store, parse and output the Cache-control HTTP header.

I do not think your HttpHdrCc class deals with the output. On the other
hand, please consider converting httpHdrCcPackInto() and
httpHdrCcUpdateStats() as well.

> -HttpHeaderFieldInfo *CcFieldsInfo = NULL;
> +/// Map an header name to its type, to expedite parsing
> +typedef std::map<String,http_hdr_cc_type> HdrCcNameToIdMap_t;
> +static HdrCcNameToIdMap_t HdrCcNameToIdMap;

HdrCcNameToIdMap should be constant so that we do not accidentally
modify it. You can make it a pointer and use similar-to-current
initialization code or try to initialize statically.

> + HdrCcNameToIdMap_t::iterator i;

The above variable can be marked as const.

> +static HdrCcNameToIdMap_t HdrCcNameToIdMap;

Consider dropping the "Hdr" prefix since you are inside a HdrCc file
already. This will also make it consistent with CcAttrs name.

> void
> +HttpHdrCc::clear()
> +{
> + mask=0;
> + max_age=-1;
> + mask=0;
> + max_age=-1;
> + s_maxage=-1;
> + max_stale=-1;
> + stale_if_error=0;
> + min_fresh=-1;
> + other.clean();
> +}

This duplicates the default constructor. Mask and max_age are cleared
twice. Consider this implementation instead (which can be inlined):

    *this = HttpHdrCc();

> + int32_t i;
> + /* build lookup and accounting structures */
> + for (i=0;i<CC_ENUM_END;i++) {

The "i" variable should be local to the loop. Please consider adding
spaces around operators.

> + assert(i==CcAttrs[i].id); /* verify assumption: the id is the key into the array */
> + HdrCcNameToIdMap[CcAttrs[i].name]=CcAttrs[i].id;

Consider computing CcAttrs[i] once instead of three times and storing it
as a local const reference.

> + assert(i==CcAttrs[i].id); /* verify assumption: the id is the key into the array */

Where do we actually use that assumption?

> - if (EBIT_TEST(cc->mask, type)) {
> + if (EBIT_TEST(mask, type)) {
> - EBIT_SET(cc->mask, type);
> + EBIT_SET(mask, type);
> - if (!p || !httpHeaderParseInt(p, &cc->max_age)) {
> + if (!p || !httpHeaderParseInt(p, &max_age)) {
> - cc->max_age = -1;
> - EBIT_CLR(cc->mask, type);
> + max_age = -1;
> + EBIT_CLR(mask, type);
> - if (!p || !httpHeaderParseInt(p, &cc->s_maxage)) {
> + if (!p || !httpHeaderParseInt(p, &s_maxage)) {

All of the above (and more) are essentially non-changes. Please consider
making "cc" equal to "this" to avoid them for the purpose of the review,
so that we can pinpoint the real changes. That hack should be removed
before the final commit, of course.

> + * Created on: Sep 2, 2011
> + * Author: Francesco Chemolli

I would recommend removing that because there are many authors for that
code. You have my permission to remove my Author tag in src/HttpHdrCc.cc
as well.

> +class HttpHdrCc
> +{
> +
> +public:
> + int32_t mask;
> + int32_t max_age;
> + int32_t s_maxage;
> + int32_t max_stale;
> + int32_t stale_if_error;
> + int32_t min_fresh;
> + String other;
> +
> + HttpHdrCc(int32_t max_age_=-1, int32_t s_maxage_=-1,

Please move public data member after public methods. It would be nice to
document them too, especially those that do not correspond to CC
directives. Also, check whether we really need all of the above,
especially mask, as public fields (more on that later below).

> + HttpHdrCc(int32_t max_age_=-1, int32_t s_maxage_=-1,
> + int32_t max_stale_=-1, int32_t min_fresh_=-1) :

Do we really all five of these constructors, including the one that
allows for implicit conversion from int32_t to HttpHdrCc? I do not think
so. Please leave just what we need and use "explicit" to prevent
implicit conversions as needed.

As far as I can see, we just need the default constructor, but my quick
check may be missing some cases:

> $ fgrep -RI httpHdrCcCreate src
> src/HttpHdrCc.cc:httpHdrCcCreate(void)
> src/HttpHdrCc.cc: HttpHdrCc *cc = httpHdrCcCreate();
> src/HttpHdrCc.cc: dup = httpHdrCcCreate();
> src/http.cc: cc = httpHdrCcCreate();
> src/protos.h:SQUIDCEXTERN HttpHdrCc *httpHdrCcCreate(void);
> src/mime.cc: reply->cache_control = httpHdrCcCreate();

> + /** reset the structure to a clear state.
> + *
> + */
> + void clear();

The "clear state" is an undefined concept. Consider a more specific
description such as:
/// reset to the after-default-construction state

(and there is probably a more elegant way of saying that).

> + * \note: internal structures are not cleaned-up beforehand.
> + * caller must explicitly clear() beforehand if he wants that
> + */
> + bool parseInit(const String &s);

It is wrong that *Init() does not initialize. If this is not your fault
and you do not want to fix it, then mark with XXX.

> + /** set the max_age value
> + *
> + * \param max_age the new max age. Values <0 clear it.
> + */
> + void setMaxAge(int32_t max_age);
> +
> + /** set the s-maxage
> + *
> + * \param s_maxage the new max age. Values <0 clear it.
> + */
> + void setSMaxAge(int32_t s_maxage);

It feels wrong to have many public fields and [only] these two methods.
We should probably hide all fields and provide the corresponding name()
and name(int32_t) accessors and setters to those fields that need them.

> === modified file 'src/HttpHeader.h'
> --- src/HttpHeader.h 2011-06-23 08:31:56 +0000
> +++ src/HttpHeader.h 2011-09-12 22:54:38 +0000
> @@ -37,6 +37,7 @@
> #include "HttpHeaderRange.h"
> /* HttpHeader holds a HttpHeaderMask */
> #include "HttpHeaderMask.h"
> +#include "HttpHdrCc.h"
>
>

Is this addition needed? Please use "class HttpHdrCc;" pre-declaration
instead if possible.

Thank you,

Alex.
Received on Tue Sep 13 2011 - 16:12:40 MDT