499852 - up network thread priority on Windows CE

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: set network thread priority

To avoid network slowdowns, it helps to up the network thread priority on Windows CE; here's a simple patch that does this.  The suggested 116 priority came from various contacts for CE6.

(This is the right thread to set, right?  We don't do IO on any other threads?)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

This is the only thread on which we do scoket I/O, but we do some file I/O from a threadpool:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStreamTransportService.cpp#476

Comment 2

[Jason Duell]

Comment on attachment 384541 [details] [diff] [review]
set network thread priority

I assume we can create a new bug/patch if we want to alter the priority of the File I/O thread, too.

Vlad:  does this fix bug 496849?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 384541 [details] [diff] [review]
set network thread priority

could you diff with more context & the -p flag next time?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/8a7eb0b510aa

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Is there a downside to up'ing the priority? We have had some complaints and bugs for Fennec that networking activity was blocking the UI. See bug 497470 for example.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Hmm, given when this was pushed and when bug 497470 was filed, it probably wasn't the cause.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Vlad - I think you should be using CeSetThreadPriorityLevel here instead of SetThreadPriorityLevel. The MSDN docs say that SetThreadPriorityLevel is only good for setting levels 248 and higher:

"Applications and device drivers should use the CeGetThreadPriority and CeSetThreadPriority functions, instead of the legacy functions GetThreadPriority and SetThreadPriority. The legacy functions are still available with the same interfaces but the functions have access only to the original 8 priority levels."

http://msdn.microsoft.com/en-us/library/aa450596.aspx

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch using CeSetThreadPriority

This patch does the same thing but uses the CeSetThreadPriority function

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 387000 [details] [diff] [review]
patch using CeSetThreadPriority

Whoops -- I recreated this patch from memory and forgot that I had to use Ce*.  Looks fine, but there's a lot of other cruft in this patch (whitespace changes).  May want to land those separately to avoid obscuring the actual change in the patch (or just not include them at all, could be your editor stripping trailing whitespace automatically).

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

FWIW, the 116 value seems to be the default priority of the NDIS driver/kernel thread -- so essentially we're saying to prioritize our processing roughly the same as getting the data in the first place.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

removed the whitespace changes:
http://hg.mozilla.org/mozilla-central/rev/0471fa063e43