Closed
Bug 714286
Opened 13 years ago
Closed 13 years ago
Cycle collection log should be written to a place where the process can write to
Categories
(Core :: XPCOM, defect)
Tracking
()
VERIFIED
FIXED
mozilla12
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(2 files, 1 obsolete file)
1.99 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Also, it's a good idea to print the full path of the log file to the error console so that the user wouldn't need to search their entire file system looking for the log file. I have a patch for this.
Assignee | ||
Comment 1•13 years ago
|
||
Updated•13 years ago
|
Attachment #584968 -
Flags: review?(bugs) → review+
Comment 2•13 years ago
|
||
Could you add some kind of #ifdef or something that lets you easily revert to the old behavior of the file name generation with a recompile? I think this is an improvement for people doing a one-off CC dump, but when you are dumping every CC, and doing multiple runs, it is helpful to have them in a predictable location. The console logging is fine either way.
Assignee | ||
Comment 3•13 years ago
|
||
I'm not sure what you mean. The location is predictable, it's the temp directory. Arguably it's more predictable than before, since at least on Windows the current directory changes when you invoke things like the file picker dialog. :-)
Comment 4•13 years ago
|
||
The directory part sounds reasonable. How does tmpnam affect the file name itself?
Assignee | ||
Comment 5•13 years ago
|
||
tmpnam() returns the full path name to a temp file, something like "/tmp/mygarbage.tmp". I can walk the string back from the end to find the first directory separator, and then append cc-edges to that, so that we would end up with "/tmp/cc-edges.N.MMMM.log" instead of "/tmp/mygarbage.tmp-cc-edges.N.MMMM.log". Is that what you want?
Assignee | ||
Comment 6•13 years ago
|
||
Implements comment 5.
Attachment #584968 -
Attachment is obsolete: true
Attachment #585044 -
Flags: review?(continuation)
Comment 7•13 years ago
|
||
Comment on attachment 585044 [details] [diff] [review] Patch (v2) Review of attachment 585044 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, that's better than the current behavior for my purposes! Because you can end up with the logs being dumped in some weird subdirectory if you invoke Firefox using Mochitest.
Attachment #585044 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 8•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c6616eea53
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla12
Comment 9•13 years ago
|
||
Could you update https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump
Comment 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c2c6616eea53
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 11•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9) > Could you update > https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump https://wiki.mozilla.org/index.php?title=Performance%3ALeak_Tools&action=historysubmit&diff=382891&oldid=382832
Assignee | ||
Comment 12•13 years ago
|
||
tmpnam() is broken on Windows. :( I have a patch for this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #585589 -
Flags: review?(continuation)
Comment 14•13 years ago
|
||
Comment on attachment 585589 [details] [diff] [review] Windows fix followup Review of attachment 585589 [details] [diff] [review]: ----------------------------------------------------------------- Looking at MS's documentation, GetTempPath returns a path that ends in \, whereas I think the other code path removes the trailing directory separator. On Windows, it looks like the path will end up being something like C:\temp\\cc-edges.txt . Is that a problem?
Assignee | ||
Comment 15•13 years ago
|
||
No. (I actually tested the patch on Windows this time. ;-))
Updated•13 years ago
|
Attachment #585589 -
Flags: review?(continuation) → review+
Comment 16•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/06de54d7d7b4
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
Verified Fixed using latest hourly m-c win32 build on Win7 x64 cset: https://hg.mozilla.org/mozilla-central/rev/0eec6ba6a87a today's m-i -> m-c merge
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•