[squid-dev] [PATCH] convert Delay Pools classes to use MEMPROXY_CLASS

[Alex Rousskov]

On 01/21/2017 06:42 PM, Amos Jeffries wrote:

> This patch converts all the delay pools classes which were providing the
> new/delete operators to using MEMPROXY_CLASS instead. So each class in
> separately accounted for and we get a better view of allocation stats
> and behaviours from the mgr:mem report.


You went beyond those desirable changes. For example:

> -    theBucket = new DelayTaggedBucket(aTag);

>      DelayTaggedBucket::Pointer const *existing = theTagged->buckets.find(theBucket, DelayTaggedCmp);

The patched code calls buckets.find() with a nil theBucket pointer AFAICT.



> Also, add construct/destruct debugs to DelayTagged and DelayTaggedBucket
> classes so the findalive.pl script can track the lifecycles of these
> classes.

... except that it actually cannot. See below.


> +    debugs(77, 3, "destruct, this=" << (void*)this);

> +    debugs(77, 3, "construct, this=" << (void*)this);


Please do not introduce a yet another flavor of construction and
destruction messages:

> $ bzr grep 'struct, this' src | wc -l
> 0


The best pattern both supported by find-alive.pl and minimally
misleading about the memory state can be sketched as:

    debugs(..., "Foo constructed, this=" << static_cast<void*>(this)...
and
    debugs(..., "Foo destructing, this=" << static_cast<void*>(this)...


If you want to rely on __FUNCTION__ providing the class name in debugs()
messages (which is the right long-term approach!), then please adjust
the "guessing construction/destruction pattern" code in find-alive.pl to
support that (the attached untested patch may be a good starting point).
Current find-alive.pl does not support that modern usage:

> echo 'MemBlob.cc(56) MemBlob: constructed, this=0x304b120' | ./scripts/find-alive.pl MemBlob
> guessing construction/destruction pattern for MemBlob
> Found 0 MemBlob


After updating find-alive.pl, the best overall pattern would become

    debugs(..., "constructed, this=" << static_cast<void*>(this)...
and
    debugs(..., "destructing, this=" << static_cast<void*>(this)...


Adding a couple of macros (ugh!) or trivial manipulator classes (a
little too much for such a trivial task?) to encapsulate that pattern is
probably a good idea:

    debugs(..., Constructed(this)...
and
    debugs(..., Destructing(this)...

but do not be tempted to wrap the entire debugs() calls into macros
because some of these calls print additional object parameters. This is
why I am ending the above patterns with "..." rather than ");". While we
could add more sophisticated macros/manipulators that accommodate
parameters, it will quickly get out of hand. And hiding debugs() calls
behind a macro will also cause various headaches.


> -long DelayPools::MemoryUsed = 0;

The total provided by this global was probably quite handy/useful. If we
can compute and still print it in DelayPools::Stats(), please do so,
probably with a "Delay pools memory used: " prefix.


> -    virtual int bytesWanted (int minimum, int maximum) const {return max(minimum,maximum);}
> +    virtual int bytesWanted(int low, int high) const { return max(low,high); }

I recommend avoiding this polishing change because it makes
bytesWanted() declarations less consistent overall and is out of scope:

> src/DelayBucket.h:    int bytesWanted (int min, int max) const;
> src/DelayId.h:    int bytesWanted(int min, int max) const;
> src/DelayTagged.h:        virtual int bytesWanted (int min, int max) const;
> src/DelayUser.h:        virtual int bytesWanted (int min, int max) const;
> src/DelayVector.h:        virtual int bytesWanted (int min, int max) const;
> src/delay_pools.cc:        virtual int bytesWanted (int min, int max) const;
> src/delay_pools.cc:        virtual int bytesWanted (int min, int max) const;
> src/delay_pools.cc:        virtual int bytesWanted (int min, int max) const;


Thank you,

Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: find-alive-post-FUNCTION.patch
Type: text/x-diff
Size: 1488 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170121/cc08bf1e/attachment.patch>