Current nsINetworkService functions will be executed in one worker thread. For some long waiting time commands like dhcpRequest, we should add the capability like 'run in new thread' to those ones.

Comment on attachment 8606825 [details] [diff] [review] Bug1165776.patch Hi Fabrice, Could you please help review this patch? It spins a new thread to run the function "dhcp_request" which may take a long time. For now there is only one command which would tag with 'runInNewThread' so that I spawn a new thread for each request. When things become more complicated in the future, I'd like to use a thread pool to reduce thread creation overhead. But for now, I just make things as simple as possible. Thanks!

Comment on attachment 8606825 [details] [diff] [review] Bug1165776.patch Review of attachment 8606825 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine for now, but please file the followup to use a thread pool. ::: dom/webidl/NetworkOptions.webidl @@ +55,5 @@ > long gateway_long; // for "ifc_configure". > long dns1_long; // for "ifc_configure". > long dns2_long; // for "ifc_configure". > + > + boolean runInNewThread = false; please add a comment.

(In reply to Fabrice Desré [:fabrice] from comment #3) > Comment on attachment 8606825 [details] [diff] [review] > Bug1165776.patch > > Review of attachment 8606825 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks fine for now, but please file the followup to use a thread pool. > > ::: dom/webidl/NetworkOptions.webidl > @@ +55,5 @@ > > long gateway_long; // for "ifc_configure". > > long dns1_long; // for "ifc_configure". > > long dns2_long; // for "ifc_configure". > > + > > + boolean runInNewThread = false; > > please add a comment. Thanks Fabrice! I'll file a followup bug to track.

webidl changes need DOM peer review

Comment on attachment 8607295 [details] [diff] [review] Bug1165776 v2 (carry r+ from Fabrice's review) Hi Blake, Could you please have a webidl change review? It's only for internal use and we add a flag to control if the command should be run in a new thread or in the common worker thread. Thanks!

Comment on attachment 8607295 [details] [diff] [review] Bug1165776 v2 (carry r+ from Fabrice's review) Review of attachment 8607295 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/NetworkWorker.cpp @@ +213,5 @@ > + if (NetworkParams.mRunInNewThread) { > + // TODO: Bug 1166131 - Use a thread pool to process commands > + // tagged with 'runInNewThread'. > + nsCOMPtr<nsIThread> thread; > + NS_NewThread(getter_AddRefs(thread), runnable); This leaks the new thread. All threads must have nsIThread::Shutdown called on them. If we limit the new thread to DHCP only, we could simply create the new thread in NetworkWorker::Start and shut it down in NetworkWorker::Shutdown and not have to worry about thread pools. Also, please use NS_NewNamedThread so we can more easily identify it in debuggers and memory reporters. ::: dom/webidl/NetworkOptions.webidl @@ +60,5 @@ > + // thread. However, for some native commands like 'dhcp_request' which > + // may block the worker thread for a long time. The caller can set > + // this flag to specifiy the internal to run this command in a separate > + // thread. > + boolean runInNewThread = false; How general is this mechanism going to be? DHCP should definitely be on its own thread, but it seems like it'd be easier to simply switch on the command name and use a different thread for dhcpRequest instead of having a generic switch to run on a new thread. If you really don't like that, I'd be OK keeping this. I want to make sure, though.

Hi Blake, Sorry for late response and thanks for your review. (In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #9) > Comment on attachment 8607295 [details] [diff] [review] > Bug1165776 v2 (carry r+ from Fabrice's review) > > Review of attachment 8607295 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/NetworkWorker.cpp > @@ +213,5 @@ > > + if (NetworkParams.mRunInNewThread) { > > + // TODO: Bug 1166131 - Use a thread pool to process commands > > + // tagged with 'runInNewThread'. > > + nsCOMPtr<nsIThread> thread; > > + NS_NewThread(getter_AddRefs(thread), runnable); > > This leaks the new thread. All threads must have nsIThread::Shutdown called > on them. If we limit the new thread to DHCP only, we could simply create the > new thread in NetworkWorker::Start and shut it down in > NetworkWorker::Shutdown and not have to worry about thread pools. > I was also worried about the leak until finding out a couple of places doing the same things like [1]. But after digging a little bit more, |Shutdown| is required to call to release the resource as you said. [1] http://hg.mozilla.org/mozilla-central/file/b0a507af2b4a/toolkit/identity/IdentityCryptoService.cpp#l332 > Also, please use NS_NewNamedThread so we can more easily identify it in > debuggers and memory reporters. > > ::: dom/webidl/NetworkOptions.webidl > @@ +60,5 @@ > > + // thread. However, for some native commands like 'dhcp_request' which > > + // may block the worker thread for a long time. The caller can set > > + // this flag to specifiy the internal to run this command in a separate > > + // thread. > > + boolean runInNewThread = false; > > How general is this mechanism going to be? DHCP should definitely be on its > own thread, but it seems like it'd be easier to simply switch on the command > name and use a different thread for dhcpRequest instead of having a generic > switch to run on a new thread. > > If you really don't like that, I'd be OK keeping this. I want to make sure, > though. I have no strong reason to keep this. So I am going to use the task name to determine which thread to dispatch. There will be a function such as |NetworkUtils::ShouldRunOutOfWorkerThread| to be called by NetworkWorker. Thanks!

No longer working on this.

Firefox OS is not being worked on