Closed
Bug 1300948
Opened 8 years ago
Closed 8 years ago
Add thread identifier to LogAlloc output
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 1 obsolete file)
12.62 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
This doesn't add support for actually replaying with separate threads, but will already allow some manual analysis on the log. I'd like to get a log with this information, for example, for bug 1300173.
Attachment #8788725 -
Flags: review?(n.nethercote)
Comment 2•8 years ago
|
||
Comment on attachment 8788725 [details] [diff] [review] Add thread identifier to LogAlloc output Review of attachment 8788725 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/replace/logalloc/LogAlloc.cpp @@ +38,5 @@ > sLock.Release(); > } > > +static intptr_t > +GetThreadId() The return type is intptr_t but you cast all calls to size_t. Just return size_t instead? Or use %zd instead of %zu in the printf calls? You could likewise write a wrapper for getpid() and do something similar to avoid all the casts. ::: memory/replace/logalloc/replay/Replay.cpp @@ +509,5 @@ > continue; > } > > + /* The log contains thread ids, but we just ignore them for now. */ > + parseNumber(line.SplitChar(' ')); Add a comment that the tid is currently only used for manual analysis? ::: memory/replace/logalloc/replay/logalloc_munge.py @@ +72,5 @@ > result = result[1:] > + if ' ' in func: > + tid, func = func.split(' ', 1) > + else: > + tid = pid Add a comment that the prior version of the format lacked the tid. (I also wonder if using a regexp would be simpler here.) Alternatively, you could just not support the old format, I think this is obscure enough that nobody will have old logs lying around? Or maybe you're thinking about the possibility of getting logs from older versions of Firefox?
Attachment #8788725 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #2) > Alternatively, you could just not support the old format, I think this is > obscure enough that nobody will have old logs lying around? Or maybe you're > thinking about the possibility of getting logs from older versions of > Firefox? Older versions of Firefox can be used with replace-malloc libs from a current m-c, so this is not really what I'm after. I'm more after being able to convert the logs that I do have somewhere lying around :)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #2) > (I also wonder if using a regexp would be simpler here.) I tested, and that's about 10% slower using a regexp. It's already slow enough IMHO. I should probably rewrite it in rust ;)
Assignee | ||
Comment 5•8 years ago
|
||
Note I renamed GetThreadId, because of the possible conflict with the Windows function with the same name.
Attachment #8788725 -
Attachment is obsolete: true
Attachment #8788754 -
Flags: review?(n.nethercote)
Comment 6•8 years ago
|
||
Comment on attachment 8788754 [details] [diff] [review] Add thread identifier to LogAlloc output Review of attachment 8788754 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/replace/logalloc/LogAlloc.cpp @@ +40,5 @@ > > +static size_t > +GetPid() > +{ > + return (size_t) getpid(); size_t(getpid())? (Likewise in GetTid().)
Attachment #8788754 -
Flags: review?(n.nethercote) → review+
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c87875a0bef Add thread identifier to LogAlloc output. r=njn
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c87875a0bef
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec45cf34da3c No bug - Remove the TODO item that was implemented in bug 1300948. r=me
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec45cf34da3c
You need to log in
before you can comment on or make changes to this bug.
Description
•