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.
Support construction/destruction patterns of modern debugs()
... which use an implicit "__FUNCTION__:" scope/prefix.
Disclaimer: This patch is untested.
=== modified file 'scripts/find-alive.pl'
--- scripts/find-alive.pl 2016-06-13 23:25:45 +0000
+++ scripts/find-alive.pl 2017-01-22 03:38:41 +0000
@@ -58,42 +58,42 @@ my %Pairs = (
'cbdataInternalAlloc: Allocating (\S+)',
'cbdataRealFree: Freeing (\S+)',
],
FD => [
'fd_open.*\sFD (\d+)',
'fd_close\s+FD (\d+)',
],
IpcStoreMapEntry => [
'StoreMap.* opened .*entry (\d+) for \S+ (\S+)',
'StoreMap.* closed .*entry (\d+) for \S+ (\S+)',
],
sh_page => [
'PageStack.* pop: (sh_page\S+) at',
'PageStack.* push: (sh_page\S+) at',
],
);
if (!$Pairs{$Thing}) {
warn("guessing construction/destruction pattern for $Thing\n");
$Pairs{$Thing} = [
- "\\b$Thing construct.*, this=(\\S+)",
- "\\b$Thing destruct.*, this=(\\S+)",
+ "\\b$Thing:? construct.*, this=(\\S+)",
+ "\\b$Thing:? destruct.*, this=(\\S+)",
];
}
die("unsupported Thing, stopped") unless $Pairs{$Thing};
my $reConstructor = $Pairs{$Thing}->[0];
my $reDestructor = $Pairs{$Thing}->[1];
my %AliveCount = ();
my %AliveImage = ();
my $Count = 0;
while (<STDIN>) {
if (my @conIds = (/$reConstructor/)) {
my $id = join(':', @conIds);
#die($_) if $Alive{$id};
$AliveImage{$id} = $_;
++$Count unless $AliveCount{$id}++;
}
elsif (my @deIds = (/$reDestructor/)) {
my $id = join(':', @deIds);
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
