Closed Bug 398397 Opened 17 years ago Closed 17 years ago

Memory leak checking script doesn't count bytes/blocks correctly.

Categories

(NSS :: Test, defect, P2)

3.11.7
Sun
Solaris
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.9

People

(Reporter: slavomir.katuscak+mozilla, Assigned: slavomir.katuscak+mozilla)

Details

Attachments

(2 files)

There are more different messages printed when memory is leaked or in use, script is now checking only for one of them, need to check for all.
Attached patch Patch v1.Splinter Review
Attachment #283348 - Flags: review?(christophe.ravel.bugs)
Attachment #283348 - Flags: review?(nelson)
Please review this patch ASAP (2 reviews), we need to get correct numbers from all platforms from both trunk and branch.
Comment on attachment 283348 [details] [diff] [review]
Patch v1.

The parsing of the output could be optimized (by using awk for example). In the meantime, this patch seems to be good enough with the current line by line parsing.
Attachment #283348 - Flags: review?(christophe.ravel.bugs) → review+
Comment on attachment 283348 [details] [diff] [review]
Patch v1.

Slavo, add a comment here when this patch has been committed on the trunk. The comment should record the date and time of the commit, and also the version number(s) of the file(s) committed.  

When I've seen the effect of this on the trunk, I will review it for the branch.
see bug 398419 for the awk version of the parsing on Linux. It should be fairly similar on Solaris.
We need more than an overall total.  We need per-process run statistics.
I suggest that Tinderbox should publish minimum, maximum, and average 
numbers of bytes and blocks per process under test.
Priority: -- → P1
Version: 3.12 → trunk
Comment on attachment 283348 [details] [diff] [review]
Patch v1.

Need to make the awk version of this script also handle these same strings.
Attachment #283348 - Flags: review?(nelson) → review+
To clarify my comment 6, I don't mean that every time we run a process 
under DBX or valgrind, we need to output those 3 statistics. 
I mean that when it's all done, those three statistics should be output 
for the whole run, namely:
- the minimum number of bytes and blocks leaked by any process under test,
- the maximum number of bytes and blocks leaked by any process under test,
- the average number of bytes and blocks leaked by any process under test,

Added statistics that Nelson wanted in previous patch.
We are currently testing 3 different tools (selfserv, strsclnt, ocspclnt), so statistics are measured for every tool separately (there is no much sense to compare data of different processes). 
Because there are too many numbers produced (2 builds x 3 processes x 2 parameters x 3 values), I suggest to print statistics only to output.log, not directly to Tinderbox results page.

In this patch I also fixed some awk indenting and style, to not mess more styles and use only one.
Attachment #285107 - Flags: review?(nelson)
Comment on attachment 285107 [details] [diff] [review]
Statistics of leaks (min, max, avg).

Slavo, please attach to this bug a file of sample output from this patch.
Thanks.
Nelson, try to check this Tinderboex logfile (not integrated yet, only this one run was patched) and search there for string 'statistics':
http://tinderbox.mozilla.org/showlog.cgi?log=NSS/1192791720.1192817006.21707.gz

Areas related to this bug:

Selfserv statistics:
Leaked bytes: 2996 min, 3117 avg, 3178 max
Leaked blocks: 34 min, 34 avg, 34 max
Total runs: 9

Strsclnt statistics:
Leaked bytes: 3450 min, 3518 avg, 3552 max
Leaked blocks: 46 min, 48 avg, 52 max
Total runs: 240

Ocspclnt statistics:
Leaked bytes: 3461 min, 3506 avg, 3552 max
Leaked blocks: 60 min, 62 avg, 64 max
Total runs: 4

The log is relatively long and you will find those areas 4 times there, because we run there DBG and OPT build and now also versions without and with PKIX.
Comment on attachment 285107 [details] [diff] [review]
Statistics of leaks (min, max, avg).

r=nelson, on the basis of the sample output shown in comment 11.
Attachment #285107 - Flags: review?(nelson) → review+
Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.18; previous revision: 1.17
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → 3.11.9
Attachment #285107 - Flags: review?(julien.pierre.boogz)
Priority: P1 → P2
I concur that this is P2 for the 3.11 branch, but it is P1 for the trunk.
Version: trunk → 3.11.7
Nelson, it's already fixed in trunk. Now I want to merge changes also to branch, because after many changes in memleak.sh in trunk it's now more difficult to merge new trunk patches to branch.
Attachment #285107 - Flags: review?(julien.pierre.boogz) → review+
Added to branch.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.