1300948 - Add thread identifier to LogAlloc output

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Add thread identifier to LogAlloc output

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.

Comment 2

[Nicholas Nethercote [inactive]]

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?

Comment 3

[Mike Hommey [:glandium]]

(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 :)

Comment 4

[Mike Hommey [:glandium]]

(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 ;)

Comment 5

[Mike Hommey [:glandium]]

Attachment: Add thread identifier to LogAlloc output

Note I renamed GetThreadId, because of the possible conflict with the Windows function with the same name.

Comment 6

[Nicholas Nethercote [inactive]]

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().)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c87875a0bef
Add thread identifier to LogAlloc output. r=njn

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c87875a0bef

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ec45cf34da3c