Re: [RFC] Interim review: typedefs.h / protos.h splitting

On Tue, Sep 18, 2012 at 5:52 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/17/2012 07:47 AM, Kinkie wrote:
>
>> Attached you'll find an interim patch aiming at replicating for
>> typedefs.h and protos.h the same work I've done for protos.h.
>>
>> It
>> - separates structures declarattions into own header files
>
> Please try to limit the number of #includes in header files. If a header
> does not dereference a class, it does not need to include the
> corresponding class declaration in most cases. For example:

I generally do. It may have slipped me, or it may be due to some previous code.
Will fix.

>> +#include "err_type.h"
>> +#include "acl/AclNameList.h"
>> +
>> +class AclDenyInfoList {
>> +public:
>> + err_type err_page_id;
>> + char *err_page_name;
>> + AclNameList *acl_list;
>> + AclDenyInfoList *next;
>> +};
>
> The acl/AclNameList.h #include can be replaced with a forward declaration:
>

> class AclNameList;
>
> For more complex cases and especially for groups of commonly used
> related classes, a corresponding "forward.h" header or equivalent is
> appropriate.
>
> This approach will reduce the number of dependencies between .cc files,
> often significantly reducing compilation time during active development.
>
>
>> - transform typedef structs (and plain structs) into classes
>
> I wonder whether it is a good idea to add a "currently a POD" or a
> similar comment to the description of these from-struct classes that
> usually lack proper constructors, destructors, etc.?
>
> Before these changes, if I saw a "struct", I knew that it most likely
> lacked those C++ properties. Now, it would be more difficult to
> distinguish those cases because the "struct" hint is gone.
>
> Not every POD needs to be converted into a "real" class, of course, but
> we need to be aware what kind of object we are dealing with and a "POD"
> label may be helpful.

Sure.

>> - renames some types to CamelCase convention
>> - adds some (for now empty placeholders) code files for c++-ification
>> of classes (e.g. CacheMgrPasswd, CustomLog)
>
>> - implements the full set of getters/setters for class RequestFlags -
>> the method names are certainly improveable, advice is sought
>
> I recommend undoing changes that do nothing but wrap a boolean field in
> two or three methods. They just add more ink and the naming is not
> consistent across different fields (e.g., some fields are "set" others
> are "marked"; some have default set values, others do not).

The names reflect my attempt at highlighting that these flags are not
consistent: some seem to be markers that something happened. Some seem
to be requests to something at a later processing stage. The names are
my attempt at disambiguating those two cases.

> Cases are where some additional processing/logic is implemented (e.g.,
> nocache_hack) should be wrapped, but why complicate processing for the
> vast majority of fields?

One possible benefit I saw was to clarify when things are set and when
they are tested.

> If you do insist on wrapping everything, please use a consistent naming
> approach. I would recommend having one getter and one setter, both with
> the same name and no default arguments.

I'm fine either way. Please consider mine as an attempt to spark a
healthy discussion :)

> There are three other issues related to the RequestFlags change:
>
>> RequestFlags():
>> + nocache(false), ims(false), auth_(false), cachable(false),
>> + hierarchical_(false), loopdetect(false), proxy_keepalive(false), proxying_(false),
>> + refresh_(false), redirected(false), need_validation(false),
>> + fail_on_validation_err(false), stale_if_hit(false), nocache_hack(false), accelerated_(false),
>> + ignore_cc(false), intercepted_(false), hostVerified_(false), spoof_client_ip(false),
>> + internal(false), internalclient(false), must_keepalive(false), connection_auth_wanted(false), connection_auth_disabled(false), connection_proxy_auth(false), pinned_(false),
>> + canRePin_(false), authSent_(false), noDirect_(false), chunkedReply_(false),
>> + streamError_(false), sslPeek_(false),
>> + doneFollowXForwardedFor(!FOLLOW_X_FORWARDED_FOR),
>> + sslBumped_(false), destinationIPLookedUp_(false), resetTCP_(false),
>> + isRanged_(false)
>> + {}
>
> For this discussion, let's assume that we initialize
> doneFollowXForwardedFor to false like other fields (and then set it in
> the constructor body to a proper value or make a smart getter).
>
> Can you check whether GCC can optimize the above into some form of
> memset(0). I suspect it cannot, especially since correctly setting a
> single bit probably requires fetching and preserving the value of nearby
> bits. If I am right, I wonder whether we should use memset(0)
> explicitly? It feels like we are adding a lot of operations to every
> request where one operation would work just fine (I know some compilers
> will even unwrap memset for small constant size values but I do not know
> whether GCC does that).

That's a possible optimization. I wonder however what happens if
nothing is specified in the constructor list. Maybe the
initializations are silently done? Or a memset is done? I suspect it
is undefined in the standard.

> Since the old code did the same kind of bit-by-bit initialization, you
> can investigate this optimization later. However, I would still change
> doneFollowXForwardedFor initialization to follow the pattern (if nothing
> else, this not-false exception tempts others to add more initialization
> exceptions, complicating future optimizations).

I would like to explain a little bit more about this, as it is another
bit that was irking me.
Some of the processing code is full of
struct {
#if somefeature
  int somevalue;
#endif
  //...
};
int somefunction() {
#if somefeature
  process(struct.somevalue);
#endif
}

This XFF change is an attempt to spark discussion on a pattern for
getting rid of this spaghetti (in a similar way like what happened
with the initializers). The proposal is to sacrifice a bit of
space-efficiency in the data structures and using #ifdefs that cause
NOPs in the functions, hopefully containing fetures enabling in the
called code, freeing the caller from having to care.
Whether the effort is useful, I'd like us to spend some time discussing on.

>> + void clearDoneFollowXFF() {
>> + /* do not allow clearing if FOLLOW_X_FORWARDED_FOR is unset */
>> + doneFollowXForwardedFor = false || !FOLLOW_X_FORWARDED_FOR;
>> + }
>
> Something went wrong here. The "false ||" part does not do anything. I
> failed to find the corresponding old code so I am not sure what this
> should be.

Old code didn't care. It's another step at what I was trying to
obtained. The invariant is: if FOLLOW_X_FORWARDED_FOR is unset,
doneFollowXForwardedFor should always return true.

> BTW, I recommend using "Xff" spelling in CamelCase names instead of
> all-caps "XFF". We already use that approach for protocol names (e.g.,
> Icap and Ssl) and it look better IMO.

Ok, will fix.

>> - request->flags.accelerated = http->flags.accel;
>> - request->flags.sslBumped = conn->switchedToHttps();
>> - request->flags.canRePin = request->flags.sslBumped && conn->pinning.pinned;
>> - request->flags.ignore_cc = conn->port->ignore_cc;
>> + if (http->flags.accel)
>> + request->flags.markAccelerated();
>> + request->flags.setSslBumped(conn->switchedToHttps());
>> + if (request->flags.sslBumped() && conn->pinning.pinned)
>> + request->flags.allowRepinning();
>> + if (conn->port->ignore_cc)
>> + request->flags.ignoreCacheControl();
>
> I do not think your patch should change unconditional setting of a flag
> into a conditional setting of a flag. Looking at the patch alone, I
> cannot tell whether the change is correct, but even if it is correct
> now, the old code was safer/clearer because it had no conditions.
> If we remove wrappers, the above changes will not be needed.
>
> If we add wrappers, I think the code should still look condition-less,
> like this:
>
> request->flags.accelerated(http->flags.accel);
> request->flags.sslBumped(conn->switchedToHttps());

Sure.

>> - changes some linkage from C to C++
>
> Instead of replacing SQUIDCEXTERN function prefix with "extern", please
> completely remove the linkage type. Ordinary functions have external
> linkage by default in C++.

Ok; will re-run the code and fix everywhere.

>> I'd like to get some feedback before committing more effort to this;
>> also an interim merge could be useful - the patch is already more than
>> 10klines long.
>
> I think you are on the right track and the changes are useful. Avoiding
> changes outside this patch intended scope ("typedefs.h / protos.h
> splitting") such as making flag settings conditional would help minimize
> contention.
>
> I am OK with committing some of the changes as long as they are
> self-contained and complete. We should balance that with the number of
> conflicts these commits will create (with all the renaming going on).
> However, if you commit partial changes, please make sure that all major
> changes are committed to one [future] Squid version (e.g., all in v3.3
> or all in v3.4). This will minimize porting pains for pending patches.

I'm not sure I understand what you mean here.. I'm generally sticking
to trunk so far.

Thanks!

-- 
    /kinkie