724368 - telemetry for number of threads

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

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

Comment 1

[:Irving Reid (No longer working on Firefox)]

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)?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Exposing the maximal number of threads to C++.

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

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Exposing the maximal number of threads to Telemetry

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Uploading the result

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

Comment 6

[Nathan Froyd [:froydnj]]

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)?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: 2. Exposing the maximal number of threads to Telemetry, v2

Fixed, thanks.

Comment 8

[Nathan Froyd [:froydnj]]

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 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: 3. Uploading the result, v2

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 731158 [details] [diff] [review]
3. Uploading the result, v2

Review of attachment 731158 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

Comment 11

[Doug Turner (:dougt)]

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.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Exposing the maximal number of threads to C++, v2

Switching everything to uint32_t for consistency.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

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

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: 1. Exposing the maximal number of threads to C++, v3

Same one, with a typo fix.

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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