Closed
Bug 1069689
Opened 10 years ago
Closed 10 years ago
A few minor processSingleLeakFile cleanups
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: mccr8, Assigned: mccr8)
Details
Attachments
(3 files, 1 obsolete file)
1.24 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
I don't know if there's some weird reason to do this in Python, but this regexp is just used with a search so a leading .* isn't useful. Also the ? is redundant as far as I can tell.
Attachment #8492247 -
Flags: review?(jmaher)
Assignee | ||
Comment 2•10 years ago
|
||
This is kind of a silly patch but it annoyed me a little.
Attachment #8492249 -
Flags: review?(jmaher)
Assignee | ||
Comment 3•10 years ago
|
||
The code that decides on the name of the leak log file is here: http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcnt.cpp#736 The process type PID format is not used when it is a default process, ergo if the file name doesn't match the PID format it is a default process. This just makes the output a little nicer, and avoids some annoying None checking.
Attachment #8492253 -
Flags: review?(jmaher)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8492253 -
Attachment is obsolete: true
Attachment #8492253 -
Flags: review?(jmaher)
Attachment #8492284 -
Flags: review?(jmaher)
Comment 5•10 years ago
|
||
Comment on attachment 8492247 [details] [diff] [review] part 1 - .*? at the start of a search regexp isn't useful. Review of attachment 8492247 [details] [diff] [review]: ----------------------------------------------------------------- pending my question, r=me ::: build/automationutils.py @@ +330,3 @@ > if leakFileBase[-4:] == ".log": > leakFileBase = leakFileBase[:-4] > + fileNameRegExp = re.compile(r"_([a-z]*)_pid\d*.log$") are there any concerns with platform specific leak logs? android, windows, b2g, etc.?
Attachment #8492247 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Well, for part 1, my patch should not affect the behavior at all. In general, the one weird platform specific thing that I'm aware of is that B2G has to copy the leak log off of the device, and it copies it into a temp file (from a hard-coded file name on the device), so you end up with the default process log being copied into some bizarrely named file. However, there's no attempt to copy leak logs off the device from any other process, so that's not a problem yet. Whatever future Marionette code has to deal with that will need to copy things into a file matching the format here, but it should probably do that anyways to produce useful messages.
Comment 7•10 years ago
|
||
Comment on attachment 8492249 [details] [diff] [review] part 2 - Don't generate a regexp then throw it away. Review of attachment 8492249 [details] [diff] [review]: ----------------------------------------------------------------- much much better!
Attachment #8492249 -
Flags: review?(jmaher) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8492284 [details] [diff] [review] part 3 - Make 'default' the default process type. Review of attachment 8492284 [details] [diff] [review]: ----------------------------------------------------------------- are there any concerns with integration into tbpl with this? I assume this might change some bugs that have leaks as the error signature will change.
Attachment #8492284 -
Flags: review?(jmaher) → review+
Comment 9•10 years ago
|
||
Can you add a comment as to where we expect the log and what assumptions we have about the name and what is in it? This is sort of backfilling a lack of documentation.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #8) > are there any concerns with integration into tbpl with this? I assume this > might change some bugs that have leaks as the error signature will change. Hmm, that's true. I think it will still be close enough. Ryan, do you think it will be a problem for starring if failures like TEST-UNEXPECTED-FAIL | leakcheck | 549403 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BasicContainerLayer, ...) become TEST-UNEXPECTED-FAIL | leakcheck | default process: 549403 bytes leaked (AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BasicContainerLayer, ...)
Flags: needinfo?(ryanvm)
Comment 11•10 years ago
|
||
Shouldn't make a difference. The search term TBPL would use for your example would be "AsyncLatencyLogger, AsyncStatement, AtomImpl, BackstagePass, BasicContainerLayer, ..." so I don't forsee anything changing. https://mxr.mozilla.org/webtools-central/source/tbpl/php/inc/AnnotatedSummaryGenerator.php#85
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 12•10 years ago
|
||
Joel, I could also just make it so the process string stays the same as it is now when the processType is default. So, something like: if processType == "default": processString = "" else: processString = " %s process:" % processType Then changing the output can be done separately if we decide that's a good idea. I'm on the fence as to whether spamming "default process" everywhere is an improvement or not.
Comment 13•10 years ago
|
||
If there is no concern from the sheriffs, this helps us explicitly call out which process is leaking. That seems like a good thing as we start dealing with e10s everywhere. is 'default' the same as 'main' ?
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #13) > If there is no concern from the sheriffs, this helps us explicitly call out > which process is leaking. That seems like a good thing as we start dealing > with e10s everywhere. is 'default' the same as 'main' ? Yeah, it seems like a good idea to specify the process type, but I don't know how useful "default" necessarily is to somebody trying to understand. But I suppose it is standard terminology, and the alternative is to R
Assignee | ||
Comment 15•10 years ago
|
||
...the alternative is to require people dig into the code to know that blank == default. Default is just the process type you get when you don't specify one otherwise. This includes the main process, but can also be other ones for reasons I don't entirely understand. One example is that in mochitest chrome we do some app tests which involve spinning up a whole new Gecko and doing stuff with that, and that new process is also a default process, but not the main process we're testing with.
Assignee | ||
Comment 16•10 years ago
|
||
Thanks for the reviews. I added a bit of a comment about the file name format. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff2c6ea2934 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b6a8b701a5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e739ab313ce7
Assignee | ||
Comment 17•10 years ago
|
||
Thanks for the fast reviews.
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8ff2c6ea2934 https://hg.mozilla.org/mozilla-central/rev/f6b6a8b701a5 https://hg.mozilla.org/mozilla-central/rev/e739ab313ce7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•