Closed Bug 1300948 Opened 3 years ago Closed 3 years ago

Add thread identifier to LogAlloc output

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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 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+
(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 :)
(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 ;)
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 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
https://hg.mozilla.org/mozilla-central/rev/4c87875a0bef
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1303232
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
You need to log in before you can comment on or make changes to this bug.