817381 - Telemetry for nsExternalAppHandler threads

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

Once bug 789932 has landed, nsExternalAppHandler downloads will use a theoretically unbounded number of IO threads. While we suspect that the number will seldom exceed 5, we should add Telemetry to ensure this.

Comment 1

[Josh Matthews [:jdm]]

https://bugzilla.mozilla.org/attachment.cgi?id=665948&action=diff is a good example of how to add a new telemetry measurement like the one required here.

Comment 2

[Sunny [:darkowlzz]]

Attachment: Added gThreadCount to count the number of threads and implemented Telemetry for the same.

Hi,
I would like to work on this bug.
Please assign it to me.

I have attached a patch of what I have worked on the bug till now.

Comment 3

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

Comment on attachment 707757 [details] [diff] [review]
Added gThreadCount to count the number of threads and implemented Telemetry for the same.

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

Good start.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +37,5 @@
>  
>  /**
> +* To keep count of the number of threads
> +*/
> +static int gThreadCount = 0;

I would put this in an anonymous namespace, to avoid any possible collision.

@@ +105,5 @@
>  
>    rv = NS_NewThread(getter_AddRefs(mWorkerThread));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  gThreadCount++;

There is no guarantee that |gThreadCount| will only ever be used from the main thread. You should protect |gThreadCount++| and |gThreadCount--| by a lock.

Comment 4

[Vladan Djeric (:vladan)]

I think this histogram would be easier to interpret if it reported the max thread count reached during a session or during a set of concurrent downloads, instead of recording every increment & decrement of gThreadCount. 

Otherwise (IIUC), initiating 5 simultaneous downloads will add values 1, 2, 3, 4, 5, 4, 3, 2, 1, 0 to the histogram. I think that would make it hard to interpret the thread count distribution from the dashboard.

Comment 5

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

(In reply to Vladan Djeric (:vladan) from comment #4)
> I think this histogram would be easier to interpret if it reported the max
> thread count reached during a session or during a set of concurrent
> downloads, instead of recording every increment & decrement of gThreadCount. 

Agreed. What is the best way to record this? Add an observer for "quit-application-granted" or "profile-change-net-teardown", perhaps?

Comment 6

[Vladan Djeric (:vladan)]

> Agreed. What is the best way to record this? Add an observer for
> "quit-application-granted" or "profile-change-net-teardown", perhaps?

It would have to be before "quit-application-granted":

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#1068

After a bit of thought, I think it might be preferable to report the max thread count reached per "concurrent download" since session-duration is highly variable. It would actually be straight-forward to implement the per-concurrent-download approach:

- Keep a "current maximum" variable around (e.g. gCurrentMax)
- Set gCurrentMax = gThreadCount when gThreadCount > gCurrentMax
- Report gCurrentMax to Telemetry when gThreadCount goes from 1 to 0, and then also reset gCurrentMax = 0

Comment 7

[Sunny [:darkowlzz]]

Attachment: Added gThreadCount to count the number of threads and implemented Telemetry for the same.

Implemented gCurrentMax with per-concurrent-download approach. 
For every |gThreadCount--|, it is checked if gThreadCount == 0. When it reaches 0, Telemetry data is updated and gCurrentMax is sent.

Yoric, please check the Mutex implementation. I am not sure if this is the right way of doing it.

Comment 8

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

Comment on attachment 709349 [details] [diff] [review]
Added gThreadCount to count the number of threads and implemented Telemetry for the same.

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

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +34,5 @@
>  #define REQUEST_RESUME_AT (1024 * 1024 * 2)
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  //// NotifyTargetChangeRunnable
>  

Note that globals are probably better off in section |Globals| than in |NotifyTargetChangeRunnable|.

@@ +38,5 @@
>  
>  /**
> +* To keep count of the number of threads
> +*/
> +static int gThreadCount = 0;

We prefer using an anonymous namespace for static variables, e.g.

namespace {
  static int gThreadCount = 0;
  // ...
}

@@ +49,5 @@
> +/**
> +* Protect gThreadCount because there is no guarantee that it will only be used
> +* from the main thread
> +*/
> +mozilla::Mutex mThreadCountLock;

Naming convention is incorrect: we prefix with |m| for "member". For more details, see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

@@ +51,5 @@
> +* from the main thread
> +*/
> +mozilla::Mutex mThreadCountLock;
> +
> +mThreadCountLock("gThreadCount.mThreadCountLock");

Ah, that one will be a little annoying to solve.

We don't really like allocating data statically like this. It can lead to all sorts of bad behaviors, depending on the dependencies of the constructor.

I realize that we should probably have made all these globals member fields of class |nsBackgroundFileSaver|. This should nicely solve the issue.

@@ +126,5 @@
> +  }
> +
> +  if (gThreadCount > gCurentMax) {
> +    gCurrentMax = gThreadCount;  
> +  }

Since you are using |gThreadCount|, this |if| should be in the block protected by |lock|.

@@ +561,5 @@
> +  // Update Telemetry data of Thread Count when the concurrent downloads are over
> +  if (gThreadCount == 0) {
> +    Telemetry::Accumulate(Telemetry::BACKGROUNDFILESAVER_THREAD_COUNT,
> +    gCurrentMax);
> +  }

As above, all uses of |gThreadCount| should be protected by the lock.
 Note that you can do it smartly without putting the call to |Telemetry::Accumulate| inside the locked section.

Also, as suggested by Vladan, you should reset |gCurrentMax| to 0.

::: toolkit/components/telemetry/Histograms.json
@@ +24,5 @@
>    },
> +  "BACKGROUNDFILESAVER_THREAD_COUNT": {
> +    "kind": "linear",
> +    "high": "20",
> +    "description": "Number of BackgroundFileSaver threads"    

Nit: No whitespace at the end of lines.

Comment 9

[Sunny [:darkowlzz]]

Attachment: Added gThreadCount to count the number of threads and implemented Telemetry for the same.

1. Repositioned global variables under their respective section in the file.
2. Created an anonymous namespace with global variables in it.
3. Moved Mutex to nsExternalAppHandler.h. 
4. |gThreadCount| and |gCurrentMax| are made protected by |lock|
5. |gCurrentMax| now resets to 0 after updating Telemetry.
6. Whitespace added at the end of |BACKGROUNDFILESAVER_THREAD_COUNT| histogram in Telemetry.

I would like to know the smart way by which calling |Telemetry::Accumulate| inside the locked section can be avoided.

Comment 10

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

Comment on attachment 709384 [details] [diff] [review]
Added gThreadCount to count the number of threads and implemented Telemetry for the same.

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

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +43,5 @@
> +* To keep the maximum number of threads during concurrent downloads
> +*/
> +static int gCurrentMax = 0;
> +
> +}

I was probably unclear in my previous review. You should move |gThreadCount| and |gCurrentMax| and make them fields of class |BackgroundFileSaver|.

@@ +81,5 @@
>  , mPipeOutputStream(nullptr)
>  , mPipeInputStream(nullptr)
>  , mObserver(nullptr)
>  , mLock("BackgroundFileSaver.mLock")
> +, mThreadCountLock("gThreadCount.mThreadCountLock")

It would be clearer to call this "BackgroundFileSaver.mThreadCountLock".

@@ +556,5 @@
> +      Telemetry::Accumulate(Telemetry::BACKGROUNDFILESAVER_THREAD_COUNT,
> +      gCurrentMax);
> +      gCurrentMax = 0;
> +    }
> +  }

That's better.

Normally, there is only one |BackgroundFileSaver| instantiated on the system, so *generally*, there should be no race between calls to |Telemetry::Accumulate|.

Froydnj, could you confirm that, in the worst case, if there is a race on this |Telemetry::Accumulate|, we are just going to lose a few samples?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> Froydnj, could you confirm that, in the worst case, if there is a race on
> this |Telemetry::Accumulate|, we are just going to lose a few samples?

That's correct.

Comment 12

[Vladan Djeric (:vladan)]

Comment on attachment 709384 [details] [diff] [review]
Added gThreadCount to count the number of threads and implemented Telemetry for the same.

- In the new histogram's description, mention that the histogram represents max concurrent threads reached during a given download session
- Nitpick: I originally came up with the "gCurrentMax" variable name but maybe "gMaxConcurrentThreadCount" would be clearer. Up to you guys
- @David: When do we have multiple BackgroundFileSavers? Mixed private-browsing & regular browsing?
- Will this patch record the gCurrentMax in the saved Telemetry Ping if Firefox shuts down while downloads are going on? i.e. does NotifySaveComplete get called on shutdown + does it get called before quit-application-granted? If the answer is no, it might be ok depending on how much accuracy you want from this data

Comment 13

[Sunny [:darkowlzz]]

Attachment: Added mThreadCount and mMaxConcurrentThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

1. Moved |gThreadCount| and |gCurrentMax| to class |BackgroundFileSaver| as private fields and renamed as |mThreadCount| and |mMaxConcurrentThreadCount| respectively.
2. Made the corresponding name changes in BackgroundFileSaver.cpp.
3. Modified the description of the new histogram.

Comment 14

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

(In reply to Vladan Djeric (:vladan) from comment #12)
> - @David: When do we have multiple BackgroundFileSavers? Mixed
> private-browsing & regular browsing?
> - Will this patch record the gCurrentMax in the saved Telemetry Ping if
> Firefox shuts down while downloads are going on? i.e. does
> NotifySaveComplete get called on shutdown + does it get called before
> quit-application-granted? If the answer is no, it might be ok depending on
> how much accuracy you want from this data


I think that paolo will answer both questions better than me.

Comment 15

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

Comment on attachment 709439 [details] [diff] [review]
Added mThreadCount and mMaxConcurrentThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

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

I'll wait for confirmation from paolo before reviewing this patch.

Comment 16

[:Paolo Amadini]

(In reply to Vladan Djeric (:vladan) from comment #12)
> - @David: When do we have multiple BackgroundFileSavers? Mixed
> private-browsing & regular browsing?

There's one BackgroundFileSaver instance for each download. All downloads handled
through nsIExternalHelperAppService use a BackgroundFileSaver instance, though
not all downloads use nsIExternalHelperAppService. The privacy status of the
download is not available in this component.

> - Will this patch record the gCurrentMax in the saved Telemetry Ping if
> Firefox shuts down while downloads are going on? i.e. does
> NotifySaveComplete get called on shutdown + does it get called before
> quit-application-granted? If the answer is no, it might be ok depending on
> how much accuracy you want from this data

The answer is no. All downloads receive the request to be stopped in
"quit-application", and BackgroundFileSaver posts a cancellation request to
the worker thread, but (as far as I know) at that point the worker threads
might be terminated by the thread manager before having a chance to even
handle the notification and invoke NotifySaveComplete on the main thread.

Comment 17

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

Ok, so this means that it we will need to complicate things a little.

The way I see it:
- we probably don't care about the privacy status of the download, as we are just interested in the number of downloads, regardless of which file is being downloaded;
- we need a global lock shared by all instances of |BackgroundFileSaver| to protect |gCurrentMax|;
- add an observer for "quit-application-granted" and use it to record |gCurrentMax|.

Paolo, Vlad, does this seem correct to you? Also, do you have a suggestion on where to declare this global lock? We probably don't want it as a toplevel value and it would be a shame to create a new service just for that purpose.

Comment 18

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> - we need a global lock shared by all instances of |BackgroundFileSaver| to
> protect |gCurrentMax|;

The places where I see counters updated and telemetry recorded are main-thread
only, thus mThreadCountLock is actually unneeded. Bug 829832 is making that
explicit in the IDL documentation (by the way, that bug is landing soon, this
patch will need to be rebased).

> - add an observer for "quit-application-granted" and use it to record
> |gCurrentMax|.

By default, downloads that are stopped on shutdown are also restarted on startup,
if resuming is supported, so in that case we still have a chance to record them
during the next session. I'm not sure if the shutdown observer gives us value.

If we need more accuracy, an alternative can be to sample at regular intervals,
recording the max value reached during the previous interval. Otherwise, the
current solution is probably the most effective we can have while keeping
things very very simple.

Comment 19

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

(In reply to Paolo Amadini [:paolo] from comment #18)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> > - we need a global lock shared by all instances of |BackgroundFileSaver| to
> > protect |gCurrentMax|;
> 
> The places where I see counters updated and telemetry recorded are
> main-thread
> only, thus mThreadCountLock is actually unneeded.

Ah, good. That was my initial assumption, but for some reason I had concluded that we could have several control threads.

 Bug 829832 is making that
> explicit in the IDL documentation (by the way, that bug is landing soon, this
> patch will need to be rebased).
> 
> > - add an observer for "quit-application-granted" and use it to record
> > |gCurrentMax|.
> 
> By default, downloads that are stopped on shutdown are also restarted on
> startup,
> if resuming is supported, so in that case we still have a chance to record
> them
> during the next session. I'm not sure if the shutdown observer gives us
> value.
> 
> If we need more accuracy, an alternative can be to sample at regular
> intervals,
> recording the max value reached during the previous interval. Otherwise, the
> current solution is probably the most effective we can have while keeping
> things very very simple.

No, we don't need absolute accuracy, far from it. So let's keep the current solution.

Thanks, Paolo.

Comment 20

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

Comment on attachment 709439 [details] [diff] [review]
Added mThreadCount and mMaxConcurrentThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

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

Paolo's input has led me to alter some of the suggestions I gave you. We all live and learn, I guess.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +107,5 @@
> +    mThreadCount++;
> +    if (mThreadCount > mMaxConcurrentThreadCount) {
> +      mMaxConcurrentThreadCount = mThreadCount;  
> +    }
> +  }

So you can get rid of all this locking, but please document why you don't need it.
Also, please remove the whitespaces at the end of lines.

@@ +543,5 @@
> +      mMaxConcurrentThreadCount);
> +      mMaxConcurrentThreadCount = 0;
> +    }
> +  }
> +

Same here.

::: netwerk/base/src/BackgroundFileSaver.h
@@ +93,5 @@
>  
> +  /**
> +  * To keep count of the number of threads
> +  */
> +  static int mThreadCount = 0;

Now that these are static, we should prefix them with "s" instead of "m".

@@ +113,5 @@
>    /**
> +   * Protect gThreadCount because there is no guarantee that it will only be used
> +   * from the main thread
> +   */
> +  mozilla::Mutex mThreadCountLock;

According to Paolo, it seems that we do have a guarantee that mThreadCount is always used from the main thread. Consequently, I misled you here and we do not need this lock.

::: toolkit/components/telemetry/Histograms.json
@@ +24,5 @@
>    },
> +  "BACKGROUNDFILESAVER_THREAD_COUNT": {
> +    "kind": "linear",
> +    "high": "20",
> +    "description": "Number of maximum concurrent threads reached during a given download session"    

Nit: Could you please remove the final whitespaces at the end of the line?

Comment 21

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sMaxConcurrentThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Please review this patch.

Once Bug 829832 is fixed, I will rebase and make it ready for landing.

Comment 22

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sMaxConcurrentThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Rebased after the fix of Bug 829832 .

Comment 23

[:Paolo Amadini]

Comment on attachment 715992 [details] [diff] [review]
Added sThreadCount and sMaxConcurrentThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

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

This is on the right track! It probably needs some more testing locally, but should mostly work as expected.

I made a few comments about style, and I've a question about the histogram, but generally it looks good. David may also have additional comments.

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +133,5 @@
>  
> +  // Update Thread Count
> +  // Note: No need of any lock as it's inside main-thread
> +  sThreadCount++;
> +  if (sThreadCount > sMaxConcurrentThreadCount) {

Just put "MOZ_ASSERT(NS_IsMainThread(), <message>);" at the beginning of the function, this will make the code self-documenting so you can remove the comment about the lock.

The other comment just exactly repeats what's obvious by reading the line of code below. I sometimes find myself writing that kind of comments, then when I re-read the code I detect them because they look like they can be written even without reading the surrounding code.

A more informative comment would look something like:

"// Keep track of the highest number of threads reached, for Telemetry."

(you might also rename the other variable to sTelemetryMaxThreadCount and even this comment would become unneeded)

style nit: Please put a dot at the end of comments.

@@ +629,5 @@
>  
> +  // Update Thread Count
> +  // Note: No need of any lock as it's inside main-thread
> +  sThreadCount--;
> +  

nit: there is whitespace at the end of this line.

@@ +630,5 @@
> +  // Update Thread Count
> +  // Note: No need of any lock as it's inside main-thread
> +  sThreadCount--;
> +  
> +  // Update Telemetry data of Thread Count when the concurrent downloads are over

It may be useful to explain in more detail what's going on here and why we reset the max count to zero. Sort of a very, very short summary of what we discussed in the bug.

@@ +633,5 @@
> +  
> +  // Update Telemetry data of Thread Count when the concurrent downloads are over
> +  if (sThreadCount == 0) {
> +    Telemetry::Accumulate(Telemetry::BACKGROUNDFILESAVER_THREAD_COUNT.
> +    sMaxConcurrentThreadCount);

Typo? Also, please indent like this:

Telemetry::Accumulate(Telemetry::BACKGROUNDFILESAVER_THREAD_COUNT,
                      sMaxConcurrentThreadCount);

::: netwerk/base/src/BackgroundFileSaver.h
@@ +107,5 @@
>     */
>    nsCOMPtr<nsIBackgroundFileSaverObserver> mObserver;
>  
> +  /**
> +   * To keep count of the number of threads

"Keeps count of the number of worker threads that are currently running."
or just "Number of worker threads that are currently running."

@@ +112,5 @@
> +   */
> +  static int sThreadCount = 0;
> +
> +  /**
> +   * To keep the maximum number of threads during concurrent downloads

Also here, what "concurrent downloads" means should be explained in more detail, mentioning in which cases this is reset to 0.

::: toolkit/components/telemetry/Histograms.json
@@ +25,5 @@
>    },
> +  "BACKGROUNDFILESAVER_THREAD_COUNT": {
> +    "kind": "linear",
> +    "high": "20",
> +    "description": "Number of maximum concurrent threads reached during a given download session"

This is missing "n_buckets", but we probably need an enumerated histogram anyways (https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe). See for example the UPDATER_SERVICE_ERRORS histogram.

How was the number 20 chosen? Or, phrased as a more general question for David also, how is this data going to inform our decisions with regard to thread handling in BackgroundFileSaver? What do we feel is the threshold that indicates we need to change our threading model here?

Comment 24

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Made the suggested changes.

Comment 25

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

(In reply to Paolo Amadini [:paolo] from comment #23)
> How was the number 20 chosen? Or, phrased as a more general question for
> David also, how is this data going to inform our decisions with regard to
> thread handling in BackgroundFileSaver? What do we feel is the threshold
> that indicates we need to change our threading model here?

I don't have hard thresholds at the moment, just intuitions. My personal intuition is that if we have a sizable percentage above 5 threads, this means that we there is something wrong. Perhaps a vulnerability to download-bombs/DoS, or perhaps a bug somewhere in calling code. And if we have a sizable percentage above 20 threads, we definitely have a problem.

Comment 26

[:Paolo Amadini]

Comment on attachment 717633 [details] [diff] [review]
Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

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

Thanks for the quick update!

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +131,5 @@
>    rv = NS_NewThread(getter_AddRefs(mWorkerThread));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Update Thread Count.
> +  MOZ_ASSERT(NS_IsMainThread(), "sThreadCount not incremented from main thread");

Please move MOZ_ASSERT at the start of the function, with a generic message like "This should be called on the main thread".

Remove the "Update Thread Count" comment.

@@ +628,5 @@
>    mWorkerThread->Shutdown();
>  
> +  // Update Thread Count.
> +  MOZ_ASSERT(NS_IsMainThread(), "sThreadCount not decremented from main thread");
> +  sThreadCount--;

The same here.

@@ +631,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "sThreadCount not decremented from main thread");
> +  sThreadCount--;
> +
> +  // Update Telemetry data of Thread Count when the concurrent downloads are over,
> +  // resetting sTelemetryMaxThreadCount for next download session.

Change the comment to:

"When there are no more active downloads, we consider the download session finished.  We record the maximum number of concurrent downloads reached during the session in a telemetry histogram, and we reset the maximum thread counter for the next download session."

::: netwerk/base/src/BackgroundFileSaver.h
@@ +116,5 @@
> +   * To keep the maximum number of threads during concurrent downloads.
> +   * When there are more than one downloads taking place at the same time,
> +   * it's known as concurrent download.
> +   * This should be reset to 0 when there is no more download taking place,
> +   * that is when sThreadCount becomes 0.

Change to:

"Maximum number of worker threads reached during the current download session, used for telemetry.

When there are no more worker threads running, we consider the download session finished, and this counter is reset."

(also note the style, we leave a blank line between paragraphs)

::: toolkit/components/telemetry/Histograms.json
@@ +25,5 @@
>    },
> +  "BACKGROUNDFILESAVER_THREAD_COUNT": {
> +    "kind": "linear",
> +    "high": "20",
> +    "n_buckets": 50,

This should have "kind": "enumerated" and "n_values": 21, as per the documentation at https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe.

See the UPDATER_SERVICE_ERRORS histogram for an example.

Comment 27

[:Paolo Amadini]

After the change, please verify in about:telemetry that the histogram appears
with the values from 0 to 20, and is updated correctly after concurrent downloads.

Comment 28

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Made changes to the comments.

When tried to build with this patch applied, error was reported saying "static variables can't be initialized directly under class". These static variables are |sThreadCount| and |sTelemetryMaxThreadCount|.
So I went to BackgroundFileSaver.cpp BackgroundFileSaver constructor and initialized them there, keeping them just declared in the class. But again some errors came up. 

Finally I tried initializing them outside the class, but no change. It said, "multiple definition".

What can be done with it?

Comment 29

[Josh Matthews [:jdm]]

Comment on attachment 717970 [details] [diff] [review]
Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

>+  static int sThreadCount = 0;

Initializing inside the class definition is illegal, as you discovered. Drop the initialization, and add a definition outside of the class (in the cpp file) like

>int BackgroundFileSaver::sThreadCount = 0;

Comment 30

[Josh Matthews [:jdm]]

The multiple definition error you were getting probably happened because you put the external definition in the header file, I'm guessing.

Comment 31

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Tried Josh's suggestion and now there are no errors but, I don't see any new Histogram, with name |BACKGROUNDFILESAVER_THREAD_COUNT|, in about:telemetry. I even tried multiple downloads at a time, but still no new histogram.

Can someone check and point out any missing element in Telemetry implementation?

Comment 32

[Vladan Djeric (:vladan)]

I didn't spot anything obviously wrong. Did you rebuild your whole tree? You added a new histogram in the middle of Histograms.json, so that means all the components that use the histograms after the new one will also need to be rebuilt.

Comment 33

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

Comment on attachment 718428 [details] [diff] [review]
Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

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

I can't find anything wrong with your Telemetry probe either. Did you rebuild the full tree?

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +68,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  //// BackgroundFileSaver
>  
> +int BackgroundFileSaver::sThreadCount = 0;
> +int BackgroundFileSaver::sTelemetryMaxThreadCount = 0;

For consistency with Telemetry::Accumulate, both should in fact be uint32_t.

@@ +611,5 @@
>  nsresult
>  BackgroundFileSaver::NotifySaveComplete()
>  {
> +  MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
> +  

Nit: Could you remove these whitespaces at the beginning of the line?

::: netwerk/base/src/BackgroundFileSaver.h
@@ +109,5 @@
>  
> +  /**
> +   * Number of worker threads that are currently running.
> +   */
> +  static int sThreadCount;

For consistency with Telemetry::Accumulate, this should in fact be uint32_t.

@@ +118,5 @@
> +   *
> +   * When there are no more worker threads running, we consider the download
> +   * session finished, and this counter is reset.
> +   */
> +  static int sTelemetryMaxThreadCount;

Same here.

::: toolkit/components/telemetry/Histograms.json
@@ +25,5 @@
>    },
> +  "BACKGROUNDFILESAVER_THREAD_COUNT": {
> +    "kind": "enumerated",
> +    "n_values": 21,
> +    "description": "Number of maximum concurrent threads reached during a given download session"

Nit: "Maximum number of concurrent threads [...]"

Comment 34

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same. (5.96 KB, patch)

* Changed int to uint32_t 
* Removed whitespaces 
* Changed description

I deleted the obj/ dir and rebuild everything and it worked. Histogram appeared in about:telemetry and showed correct data. I tested it with 2 downloads at the same time and found the correct representation on histogram.

Comment 35

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

Comment on attachment 719490 [details] [diff] [review]
Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same. (5.96 KB, patch)

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

Looks good to me, with the minor change below.
For consistency, I believe that either paolo or cbiesinger are probably the best persons do the final review.

::: toolkit/components/telemetry/Histograms.json
@@ +25,5 @@
>    },
> +  "BACKGROUNDFILESAVER_THREAD_COUNT": {
> +    "kind": "enumerated",
> +    "n_values": 21,
> +    "description": "Maximum number of concurrent threads [...]"

Hey, no, I expected you to fill in the "[...]" :)

Comment 36

[Sunny [:darkowlzz]]

Attachment: Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Modified telemetry description.

Comment 37

[:Paolo Amadini]

Comment on attachment 720429 [details] [diff] [review]
Added sThreadCount and sTelemetryMaxThreadCount to count the number of maximum concurrent threads in a download session and implemented Telemetry for the same.

Looks good to me also, tested and working on Linux.

You need final review from a peer of the Necko module before this can be
pushed to the source tree.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8c25729d7a

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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