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 squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev