in bug 679498, tantek has ~1000 threads. we should add telemetry for thread count, to see how common a problem this is.

Do we want the peak count for the entire session, or do we want to sample periodically and make a histogram of peak thread counts over some time interval (that is, every n minutes we insert a histogram entry for the peak number of threads seen during the preceding n minutes)?

For the moment, we are interested in a peak count. If this peak count is worrying, it there will still be time for something more detailed. I have a C++ patch that extracts the information, and I am now looking for the most appropriate place to feed this to Telemetry. Nathan suggested using your hooks from bug 852974.

Doug, do you have the time to review this patch?

This passes tests. However, I have not found if adding a simple measurement requires some registration.

Comment on attachment 731115 [details] [diff] [review] 2. Exposing the maximal number of threads to Telemetry Review of attachment 731115 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the IDL change and similar change to the .cpp. ::: toolkit/components/telemetry/nsITelemetry.idl @@ +88,5 @@ > + /** > + * A number representing the highest number of concurrent threads > + * reached during this session. > + */ > + readonly attribute jsval maximalNumberOfConcurrentThreads; Why not just make this an int (or long, if that's what it needs to be)?

Fixed, thanks.

Comment on attachment 731116 [details] [diff] [review] 3. Uploading the result Review of attachment 731116 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ +34,5 @@ > const PR_CREATE_FILE = 0x8; > const PR_TRUNCATE = 0x20; > const RW_OWNER = 0600; > > +const NUMBER_OF_THREADS_TO_LAUNCH = 100; I think you could get by with fewer threads. @@ +48,5 @@ > > +// Launch a number of threads, just for the sake of measuring the total number > +// of threads concurrently executing. > +for (let i = 0; i < NUMBER_OF_THREADS_TO_LAUNCH; ++i) { > + Services.tm.newThread(0); For sanity's sake, I think you want to measure the number of threads that actually got launched and use that below to avoid test failures induced by low memory or other conditions. Catching failure of the newThread call would be part of that. Please also move this into the main test function; I don't think there's a good reason for this to be at top level. Bonus points if you add code to clean up these launched threads as well.

Comment on attachment 731158 [details] [diff] [review] 3. Uploading the result, v2 Review of attachment 731158 [details] [diff] [review]: ----------------------------------------------------------------- r=me.

Comment on attachment 731114 [details] [diff] [review] 1. Exposing the maximal number of threads to C++. Review of attachment 731114 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/nsThreadManager.h @@ +43,5 @@ > nsThread *GetCurrentThread(); > > + // Returns the maximal number of threads that have been in existence > + // simultaneously during the execution of the thread manager. > + uint GetHighestNumberOfThreads(); Is there any reason for uint here? I think other code uses uint32_t in xpcom.

Switching everything to uint32_t for consistency.

Try: https://tbpl.mozilla.org/?tree=Try&rev=5fda2170626a

Same one, with a typo fix. Try: https://tbpl.mozilla.org/?tree=Try&rev=bd1e7da1f2ae

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb6cb545326 https://hg.mozilla.org/integration/mozilla-inbound/rev/532c3e448de0 https://hg.mozilla.org/integration/mozilla-inbound/rev/c549149d2b9e

https://hg.mozilla.org/mozilla-central/rev/4fb6cb545326 https://hg.mozilla.org/mozilla-central/rev/532c3e448de0 https://hg.mozilla.org/mozilla-central/rev/c549149d2b9e