1471280 - Do not start/stop nsHostResolver threads

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

Following bug 1426019, nsHostResolver uses regular nsIThread for it's resolver workers. These threads don't really need to be shutdown, as we can just post events to them as necessary.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1471280 - Use nsThreadPool for DNS resolver threads

Instead of creating and deleteing each thread, we use a nsThreadPool with a max of 8 resolver threads.
Whereas before each thread would run ThreadFunc exactly once then shut down, the threads may now remain active for a while. During this time we may post another task(runnable) to the thread.

Review commit: https://reviewboard.mozilla.org/r/254798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254798/

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1471280 - Add new pref for how much longer resolver threads should remain idle

The new pref is "network.dns.resolver-thread-extra-idle-time-seconds"
The default is 60 seconds. This means that threads will stay idle for an extra 60 seconds, after which they are shutdown.
Setting the pref to 0 would preserve the behaviour before the threads were swiched to use nsThreadPool - meaning that they would be shutdown immediately after ThreadFunc completes.
Setting the pref to -1 would keep the threads alive forever.

Review commit: https://reviewboard.mozilla.org/r/254800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254800/

Comment 3

[Daniel Stenberg [:bagder]]

Comment on attachment 8989837 [details]
Bug 1471280 - Use nsThreadPool for DNS resolver threads

https://reviewboard.mozilla.org/r/254798/#review261766

Comment 4

[Daniel Stenberg [:bagder]]

Comment on attachment 8989838 [details]
Bug 1471280 - Add new pref for how much longer resolver threads should remain idle

https://reviewboard.mozilla.org/r/254800/#review261770

::: modules/libpref/init/all.js:2046
(Diff revision 1)
>  // Contols whether or not "localhost" should resolve when offline
>  pref("network.dns.offline-localhost", true);
>  
> +// Defines how much longer resolver threads should stay idle before are shut down.
> +// A negative value will keep the thread alive forever.
> +pref("network.dns.resolver-thread-extra-idle-time-seconds", 60);

My gut feeling says we could use a little longer default timeout so that the threads would mostly linger and stick around while a user is actually active and only end them after a longer pause (like perhaps a 300 seconds timeout instead). I'm thinking 60 seconds is just a long page load and reading a page for a while.

Again, not based on science at all. Feel free to ignore if you disagree.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to Daniel Stenberg [:bagder] from comment #4)
> > +pref("network.dns.resolver-thread-extra-idle-time-seconds", 60);
> 
> My gut feeling says we could use a little longer default timeout so that the
> threads would mostly linger and stick around while a user is actually active
> and only end them after a longer pause (like perhaps a 300 seconds timeout
> instead). I'm thinking 60 seconds is just a long page load and reading a
> page for a while.
> 
> Again, not based on science at all. Feel free to ignore if you disagree.

Well, this actually increases the idle time by 60 seconds above what it is now:
LongIdleTimeoutSeconds  300
ShortIdleTimeoutSeconds 60 

I want to run this for a couple of weeks, then I'm considering increasing this to forever™, according to bug 1470907 comment 7.
This could show a slight improvement in DNS telemetry, since threads don't have to be spun up now. But I have no idea how the extra threads would affect resource consumption on Android for example.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db553cb4218d
Use nsThreadPool for DNS resolver threads r=bagder
https://hg.mozilla.org/integration/autoland/rev/d7788f2fd8a2
Add new pref for how much longer resolver threads should remain idle r=bagder

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/db553cb4218d
https://hg.mozilla.org/mozilla-central/rev/d7788f2fd8a2