Closed Bug 519570 Opened 11 years ago Closed 10 years ago

Leaktest + Electrolysis: count leaks for each subprocess

Categories

(Testing :: Mochitest, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: jgriffin)

References

Details

Attachments

(2 files, 4 obsolete files)

The leaktest framework should be able to measure and warn about leaks from each process. Currently the leak output is intermingled and is likely to cause problems.
Currently, all leaks are dumped into runtests_leaks.log (from setting XPCOM_MEM_BLOAT_LOG).  I'm thinking each child process could use its own log, named [XPCOM_MEM_BLOAT_LOG]_childX.log, where X starts at 0 and increments as needed.  It would be easy then to adapt processLeakLog() to read all the log files and write out the data in a reasonable way.

The logs are written in nsTraceRefcntImpl.cpp.  It seems it can be easily adapted per the above.  It looks like I could use XRE_GetProcessType() to determine whether I was in a child process or not. 

Does this sound like a good approach?
Yes.
Depends on: 524778
Attached patch patch (obsolete) — Splinter Review
This patch causes separate leak logs to be spun off for each child process.  Will post a separate patch to handle the python end of the arrangement.
Assignee: nobody → jgriffin
Status: NEW → ASSIGNED
Attachment #408915 - Flags: review?
Attachment #408915 - Flags: review? → review?(benjamin)
Attached patch python patch (obsolete) — Splinter Review
Here's the python half of the patch.  

In the log, this adds a process identifier to the first line, e.g.

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, Primary Process
 - or -
== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, Child Process 0

We could also add this process identifier into the TEST-UNEXPECTED-FAIL lines, but this may add too much clutter:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | child 0 | leaked 1 instance of nsStringBuffer with size 8 bytes
Attachment #409176 - Flags: review?(ted.mielczarek)
Comment on attachment 408915 [details] [diff] [review]
patch

>diff --git a/xpcom/base/nsTraceRefcntImpl.cpp b/xpcom/base/nsTraceRefcntImpl.cpp

>     else {
>-      FILE *stream = ::fopen(value, "w");
>+      FILE *stream;
>+      int fnamesize = strlen(value) + 20;
>+      char* fname = (char*)PR_Malloc(fnamesize + 1);
>+      strncpy(fname, value, fnamesize);

Better memory management with strings:
nsCAutoString fname(value);

if (!default) {
  fname.AppendLiteral("_child");
  fname.AppendInt(counter);


>+#ifdef MOZ_IPC
>+      if (XRE_GetProcessType() != GeckoProcessType_Default) {
>+        FILE *exists = NULL;

This whole block seems complicated and unfortunate. It's like nsIFile.createUnique but it's still subject to race conditions. So I'd prefer that you either use nsIFile.createUnique, or use an idenfitier which is actually unique per-process, for example the PID. Unfortunately we don't seem to have a cross-platform way of getting the PID, but I'd be fine if you just did

#ifdef XP_WIN
#include <process.h>
#define getpid _getpid
#else
#include <unistd.h>
#endif

And then used getpid. This has the added advantage that you can associate the leak log with the PID.
Attachment #408915 - Flags: review?(benjamin) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Thanks for the review, bsmedberg.  Patch updated per your comments.  Python patch will be updated too, to match.
Attachment #408915 - Attachment is obsolete: true
Attachment #409176 - Attachment is obsolete: true
Attachment #409429 - Flags: review?(benjamin)
Attachment #409176 - Flags: review?(ted.mielczarek)
Attached patch patch v3Splinter Review
This version uses the process type string instead of the generic "child" in the logfile name, and adds the PID and process type to the header inside the log.
Attachment #409429 - Attachment is obsolete: true
Attachment #409757 - Flags: review?(benjamin)
Attachment #409429 - Flags: review?(benjamin)
Attached patch python patch v2 (obsolete) — Splinter Review
Here's the python half.  This does add the process type and PID to the TEST-UNEXPECTED-FAIL lines, for non-default processes, e.g:

TEST-UNEXPECTED-FAIL | plugin process 552 | automationutils.processLeakLog() | l
eaked 8 bytes during test execution
Attachment #409772 - Flags: review?(ted.mielczarek)
Attachment #409757 - Flags: review?(benjamin) → review+
Comment on attachment 409772 [details] [diff] [review]
python patch v2

>diff --git a/build/automationutils.py b/build/automationutils.py

>-def processLeakLog(leakLogFile, leakThreshold = 0):
>+def processLeakLog(leakLogFile, leakThreshold = 0, checkChildren = False):

No reason to default to false: just remove this parameter and always checkChildren.


>+  (leakLogFileDir, leakFileBase) = os.path.split(leakLogFile)
>+  pidRegExp = ".*?_([a-z]*)_pid(\d*)$"
>+  if checkChildren and leakFileBase[-4:] == ".log":
>+    leakFileBase = leakFileBase[:-4]
>+    pidRegExp = ".*?_([a-z]*)_pid(\d*).log$"

You will be better off if you precompile the regexp objects. Also, please use r'' quoting for regexps: the \d in this case is being interpreted as a string quote, not a regexp quote!

  pidRegExp = re.compile(r'.*?_([a-z]*)_pid(\d*)$')

>+  for fileName in os.listdir(leakLogFileDir):

This re-indent makes the entire file get pretty indented. Perhaps the logic for processing one specific file could be separated out into a function?
Attachment #409772 - Flags: review?(ted.mielczarek) → review-
Attached patch python patch v3Splinter Review
Attachment #409772 - Attachment is obsolete: true
Attachment #411294 - Flags: review?(benjamin)
Attachment #411294 - Flags: review?(benjamin) → review+
Pushed as http://hg.mozilla.org/projects/electrolysis/rev/246ea55e42c3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 535106
You need to log in before you can comment on or make changes to this bug.