200893 - Timeline Service is not thread aware

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Mark Hammond [:markh] [:mhammond]]

The timeline service is very useful.  ActiveState's Komodo is getting excellent
value from it.  However, it is heavily threaded which made the results hard to
interpret.

Attaching a patch that makes the service thread aware.  It keeps counters,
indentation etc per-thread rather than globally, and dumps a thread-id with
every line.  Thus, indentation and counters are always accurate for their thread
(we use a simple Python script to post-process the log and split it into
threads).  Also uses NS_IMPL_THREADSAFE_ISUPPORTS1 to make it truly thread-safe.

The patch manages to throw away all the locks!  Thread Local Storage is used, so
each thread has private data; no contention is possible. Startup contention is
managed via PRCallOnce.  Each thread has the capacity to be disabled discretely
from the main service, but this is not exposed.

Initialization has been rationalized somewhat, as a thread's "initTime" is the
first call for that thread, rather than when the service is enabled.

Note this patch also includes the trivial patch in bug 119136 - I forgot to
remove it before working on this.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Thread aware timeline service

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Thread aware timeline service

Previous patch threw an assertion when service disabled.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 119606 [details] [diff] [review]
Thread aware timeline service

adding to my queue.

the first nit is that I don't like the name of the new method :-/ Maybe just
markTimerWithComment or something...

more later.

Comment 4

[Alec Flett]

Comment on attachment 119606 [details] [diff] [review]
Thread aware timeline service

+    PR_snprintf(tbuf, sizeof tbuf, "%05d.%03d (%08p): %s\n",
+		 secs, msecs, PR_GetCurrentThread(), pbuf);


maybe consider increasing the size of tbuf now that you're adding an extra
field to the output?
r/sr=alecf

Comment 5

[Doug Turner (:dougt)]

mark, I think am okay with the patch (with or without the method name change --
after all, this interface can be changed later if we really want).  Do you want
to check this in or shall I?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Fixed in nsTimelineService.cpp:3.10 and nsITimelineService.idl:3.9

markTimeWithComments() was used, and I threw an extra 50 bytes for that buffer.

Thanks!