773518 - netwerk\cache\nsDeleteDir.cpp should lower IO prioritization on Windows Vista+ and OS X

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Brian R. Bondy [:bbondy]]

Before Windows Vista, even if you have a low priority thread, the IO requests that it issues can still flood normal IO.  Since Windows Vista though, you can control thread priority.  

You can use SetThreadPriority w/ THREAD_MODE_BACKGROUND_BEGIN and THREAD_MODE_BACKGROUND_END as of Vista to mark IO from that thread as low priority.

The code that is used to clear inconsistent cache is often run and deletes files over time.  These IO requests should be marked with low priority, or better yet we should just mark the whole deleter thread with low IO priority.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Comment 2

[Jason Duell]

Comment on attachment 642452 [details] [diff] [review]
Patch v1.

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

::: xpcom/base/nsWindowsHelpers.h
@@ +101,5 @@
> +// priority.  This helper is used to set the current thread to low IO priority
> +// for the lifetime of this object.
> +// Note: You can only use this low priority IO setting within the context of
> +// the current thread.
> +class nsAutoLowPriorityIOThread

I'm not a fan of the specific name/API here.  This object isn't a "Thread", it's an "Interval", really, so I'd go with 'nsAutoLowPriorityIOInterval'.  But that's not my call--you need an xpcom peer to review the nsWindowsHelpers.h changes.  

Note that the DeleteDir thread doesn't need to be reset back to regular priority--you could just set it to THREAD_MODE_BACKGROUND_BEGIN and never reset it again (assuming windoze is happy with that).   So a non-interval API might be more flexible.  But it works fine for the case here.

Finally, I poked around and there seem to be OSX and Linux equivalents for this functionality, and it would be really nice to have them (ideally using a single cross-platform API, in NSPR or somewhere else):

  http://linux.die.net/man/2/ioprio_set

  https://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man3/setiopolicy_np.3.html

So if you want super-bonus points, implement OSX/Linux too.  Otherwise file a followup bug for them, and you've got my +r for the necko bits here once you get signoff from an xpcom peer.

Comment 3

[Jason Duell]

WTC:  would you be interested in taking an NSPR patch that allows thread I/O priority to be lowered?  We've got APIs for Windows/OSX/Linux, and other platforms could be no-ops for starters and added incrementally when APIs are available.

If not we'll probably just land this in Firefox.

Comment 4

[Brian R. Bondy [:bbondy]]

I'm not opposed to doing this on other platforms too.  Maybe we could do an XPCOM object or something and have implementations only in Windows and OSX and return NS_ERROR_NOT_IMPLEMENTED otherwise?

For the Linux API you mentioned ioprio_set I don't think you can use it per thread. 

Note that if we add it into PR_SetThreadPriority it will only work in Windows when the passed in thread is the current thread. 

> I'm not a fan of the specific name/API here.  
> This object isn't a "Thread", it's an "Interval"

It's really a thread pseudo-state that lasts for the lifetime of the object. Typically for RAII classes I believe we use Auto as the indicator of this aspect.

> Note that the DeleteDir thread doesn't need to be reset back to regular priority

I think, but I can't find documentation on it, that Windows also lowers the priority of the thread to idle-state. So having the thread state only for this interval I think would be best.

Comment 5

[Brian R. Bondy [:bbondy]]

If we decide to go with a cross platform solution btw, I'll post a new bug for that work and mark it as a blocker for this bug.  This bug would have just the netwerk code.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> I'm not opposed to doing this on other platforms too.  Maybe we could do an
> XPCOM object or something and have implementations only in Windows and OSX
> and return NS_ERROR_NOT_IMPLEMENTED otherwise?

We don't need XPCOM for this. We can just have a regular function that returns an nsresult, with the implementation #ifdef'd.

Also, I don't know if it is a good idea to put it in NSPR; some Mozillians want to minimize NSPR dependencies in the hopes of removing some/most/all of the NSPR dependencies at some point.

Comment 7

[Brian R. Bondy [:bbondy]]

The reason I suggested XPCOM is so we could use it from JS code at some point in the future.  But ya we could start with that and change to an XPCOM implementation if we ever actually find a need.  Would the implementation live in xpcom\base?

Comment 8

[Brian R. Bondy [:bbondy]]

Or maybe just put it in netwerk\cache for now like nsDeleteDir?

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> The reason I suggested XPCOM is so we could use it from JS code at some
> point in the future.  But ya we could start with that and change to an XPCOM
> implementation if we ever actually find a need.  Would the implementation
> live in xpcom\base?

netwerk/cache or xpcom/io. I don't care which.

Comment 10

[Jason Duell]

xpcom/io seems better, as there may be other code that'll want to use IO priority setting, and it doesn't logically belong in cache.

> For the Linux API you mentioned ioprio_set I don't think you can use it per thread. 

You're right--that's sad.

Don't get too held up on the niceties of XPCOM or NSPR for now--might as well just land something and get this working for OSX/Windows--it'll be nice to have.  Thanks for looking into this.

Comment 11

[Honza Bambas (:mayhemer)]

xpcom/threads?  nsThreadUtils.h?

Comment 12

[Brian R. Bondy [:bbondy]]

I'll throw it in xpcom/glue/nsThreadUtils.h, thanks for the suggestion.

Comment 13

[Wan-Teh Chang]

(In reply to Jason Duell (:jduell) from comment #3)
> WTC:  would you be interested in taking an NSPR patch that allows thread I/O
> priority to be lowered?  We've got APIs for Windows/OSX/Linux, and other
> platforms could be no-ops for starters and added incrementally when APIs are
> available.

jduell: if these functions can be implemented on the three main platforms
(Linux, Mac, and Windows), then it is fine to add them to NSPR.  You can
add them to xpcom, too.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Netwerk code - Patch v2.

- Removed Thread from the name of the type. 
- Removed platform specific code from within /netwerk
- Split out stuff in /xpcom/glue for another reviewer

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Low priority IO thread state code - Patch v1.

Comment 16

[Jason Duell]

Comment on attachment 643165 [details] [diff] [review]
Netwerk code - Patch v2.

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

Looks good

::: netwerk/cache/nsDeleteDir.cpp
@@ +179,5 @@
>    dirList = static_cast<nsCOMArray<nsIFile> *>(arg);
>  
>    bool shuttingDown = false;
> +
> +  // Intentional extra braces to control variable sope.

s/sope/scope/

Comment 17

[Jason Duell]

Comment on attachment 643166 [details] [diff] [review]
Low priority IO thread state code - Patch v1.

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

::: xpcom/glue/nsThreadUtils.h
@@ +494,5 @@
>    void operator=(const nsThreadPoolNaming &) MOZ_DELETE;
>  };
>  
> +// Low priority threads do not imply IO priority issued by that thread in
> +// Windows.  This helper is used to set the current thread to low IO priority

"Thread priority in most operating systems affects scheduling, not IO. This helper..."

Comment 18

[Brian R. Bondy [:bbondy]]

This is passing tests from a push to try.

Comment 19

[Benjamin Smedberg]

Comment on attachment 643166 [details] [diff] [review]
Low priority IO thread state code - Patch v1.

>+// Low priority threads do not imply IO priority issued by that thread in
>+// Windows.  This helper is used to set the current thread to low IO priority
>+// for the lifetime of the created object.
>+// Note: You can only use this low priority IO setting within the context of
>+// the current thread.

Please make this a javadoc-style comment:

/**
 * ...
 */

>+class nsAutoLowPriorityIO

Please make this NS_STACK_CLASS... for now I can only think of reasons to use this as an automatic var.

>+{
>+public:
>+  nsAutoLowPriorityIO();
>+  ~nsAutoLowPriorityIO();
>+

The rest of this class should be private, I think. And please use standard member naming "mLowIOPrioritySet" and "mOldPriority".

r=me with those changes.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: XPCOM nsThreadUtils - Patch v2. Implemented nits

Comment 21

[Brian R. Bondy [:bbondy]]

I'll land this once Bug 774140 gets to r+.

Comment 22

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0a9061addc84
http://hg.mozilla.org/integration/mozilla-inbound/rev/f544b4af9a0f

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0a9061addc84
https://hg.mozilla.org/mozilla-central/rev/f544b4af9a0f

Comment 24

[Jason Duell]

\o/  thanks Brian!

Comment 25

[Brian R. Bondy [:bbondy]]

Thanks for the reviews!