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

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Trunk
mozilla17
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p2])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 642452 [details] [diff] [review]
Patch v1.
Attachment #642452 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Depends on: 774140
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.
Attachment #642452 - Flags: review?(jduell.mcbugs) → feedback+
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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(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.
(Assignee)

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
Or maybe just put it in netwerk\cache for now like nsDeleteDir?
(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.
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.
xpcom/threads?  nsThreadUtils.h?
(Assignee)

Comment 12

5 years ago
I'll throw it in xpcom/glue/nsThreadUtils.h, thanks for the suggestion.
(Assignee)

Updated

5 years ago
Summary: netwerk\cache\nsDeleteDir.cpp should use Windows Vista IO prioritization to lower its IO priority → Bug 773518 - netwerk\cache\nsDeleteDir.cpp should lower IO prioritization on Windows Vista+ and OS X
(Assignee)

Updated

5 years ago
Summary: Bug 773518 - netwerk\cache\nsDeleteDir.cpp should lower IO prioritization on Windows Vista+ and OS X → netwerk\cache\nsDeleteDir.cpp should lower IO prioritization on Windows Vista+ and OS X

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 643165 [details] [diff] [review]
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
Attachment #642452 - Attachment is obsolete: true
Attachment #643165 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Attachment #643165 - Attachment description: Patch v2. → Netwerk code - Patch v2.
(Assignee)

Comment 15

5 years ago
Created attachment 643166 [details] [diff] [review]
Low priority IO thread state code - Patch v1.
Attachment #643166 - Flags: review?(benjamin)
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/
Attachment #643165 - Flags: review?(jduell.mcbugs) → review+
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..."
Attachment #643166 - Flags: feedback+
(Assignee)

Comment 18

5 years ago
This is passing tests from a push to try.

Updated

5 years ago
Whiteboard: [snappy] → [snappy:p2]
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.
Attachment #643166 - Flags: review?(benjamin) → review+
(Assignee)

Comment 20

5 years ago
Created attachment 645331 [details] [diff] [review]
XPCOM nsThreadUtils - Patch v2. Implemented nits
Attachment #643166 - Attachment is obsolete: true
Attachment #645331 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #645331 - Attachment description: Implemented nits → XPCOM nsThreadUtils - Patch v2. Implemented nits
(Assignee)

Comment 21

5 years ago
I'll land this once Bug 774140 gets to r+.
(Assignee)

Comment 22

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/0a9061addc84
http://hg.mozilla.org/integration/mozilla-inbound/rev/f544b4af9a0f
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/0a9061addc84
https://hg.mozilla.org/mozilla-central/rev/f544b4af9a0f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
\o/  thanks Brian!
(Assignee)

Comment 25

5 years ago
Thanks for the reviews!
Depends on: 1331171
You need to log in before you can comment on or make changes to this bug.