800532 - Use low priority IO for staged updates in Windows and OS X

Status: RESOLVED WONTFIX

(Toolkit :: Application Update, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use low priority IO on OS X

I've noticed that background updates tend to be pretty hard on my Mac. This patch lowers the io priority when doing background updates.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This should actually probably use IOPOL_SCOPE_PROCESS instead of IOPOL_SCOPE_THREAD

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 670521 [details] [diff] [review]
Use low priority IO on OS X

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

Why not do this on Windows as well?  To see the equivalent Windows code see here:
http://dxr.mozilla.org/mozilla-central/xpcom/glue/nsThreadUtils.cpp.html#l299

And yes you might as well do it at the process level instead.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2057,5 @@
> +{
> +#ifdef XP_MACOSX
> +    setiopolicy_np(IOPOL_TYPE_DISK,
> +                   IOPOL_SCOPE_THREAD,
> +                   IOPOL_THROTTLE) != -1;

nit: Remove != -1

Comment 3

[Brian R. Bondy [:bbondy]]

You can r? me with the changes by the way.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use low priority IO on OS X V2

Comment 5

[(no longer active)]

I'd like us not to do this on Windows if we can avoid that, since we don't have any evidence that this is a problem on Windows (we use CopyFile there), and we should in general opt for the updater to finish its job as soon as possible.

Comment 6

[Brian R. Bondy [:bbondy]]

What evidence do you need? If 2 IO operations are happening, one from Firefox and one from updater which runs in the background without the user knowing, which one would you prefer gets IO priority?

What are you afraid of in lowering the priority? And why should we react differently on OSX and Windows where equivalent API exist?

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 670891 [details] [diff] [review]
Use low priority IO on OS X V2

Please see Comment 2. I would be fine if you get a review from someone else though if you want to go with what you have in terms of OSX only.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 670891 [details] [diff] [review]
Use low priority IO on OS X V2

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

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2053,4 @@
>  }
>  #endif
>  
> +static void LowerIOPriority()

nit static and void should go on the previous line.

@@ +2057,5 @@
> +{
> +#ifdef XP_MACOSX
> +    setiopolicy_np(IOPOL_TYPE_DISK,
> +                   IOPOL_SCOPE_PROCESS,
> +                   IOPOL_THROTTLE);

nit: I think this would fit on one line.

Comment 9

[(no longer active)]

(In reply to comment #6)
> What evidence do you need? If 2 IO operations are happening, one from Firefox
> and one from updater which runs in the background without the user knowing,
> which one would you prefer gets IO priority?

On OS X and Linux, we copy files to a userspace buffer and then write them back to a kernel buffer.  On Windows, the data never hits the userpsace buffers, so the copy will be faster.

> What are you afraid of in lowering the priority? And why should we react
> differently on OSX and Windows where equivalent API exist?

Longer updater.exe lifetimes mean more chances of it getting killed because of some broken antivirus software, the system losing power, etc.  I don't have a concrete reason, mostly fear of the unknown.  :-)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> (In reply to comment #6)
> > What evidence do you need? If 2 IO operations are happening, one from Firefox
> > and one from updater which runs in the background without the user knowing,
> > which one would you prefer gets IO priority?
> 
> On OS X and Linux, we copy files to a userspace buffer and then write them
> back to a kernel buffer.  On Windows, the data never hits the userpsace
> buffers, so the copy will be faster.

Still the copy to userspace isn't not the part that will hurt system responsiveness.

Comment 11

[Brian R. Bondy [:bbondy]]

> On OS X and Linux, we copy files to a userspace buffer and then write them
> back to a kernel buffer.  On Windows, the data never hits the userpsace
> buffers, so the copy will be faster.

Copying data into buffers is not the slow part of an IO operation, and I'm sure it's not the reason why it was slow for Jeff in Comment 0.  If you are against lowering IO priority that seems fine with me but I really do question why OSX should be special cased.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use low priority IO on OS X V3

I can do Windows in a follow up if we think that's the right thing to do.

Comment 13

[Brian R. Bondy [:bbondy]]

Comment on attachment 670921 [details] [diff] [review]
Use low priority IO on OS X V3

I'm personally not OK with special casing OSX in this regard as implied in Comment 7.  I would be fine if you get a review from someone else though if you want to go with what you have in terms of OSX only.

Comment 14

[Brian R. Bondy [:bbondy]]

Also, you attached the wrong patch for review by the way.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use low priority IO on OS X and Windows

I tend to agree that running at lower priority on Windows is the right thing to do. We shouldn't have updating at priority above what the user is actually trying to do on their computer.

Comment 16

[(no longer active)]

Fine!

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 670950 [details] [diff] [review]
Use low priority IO on OS X and Windows

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

r=me with changes, please push to try before landing.  Thanks for adding the Windows code.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +2061,5 @@
> +  OSVERSIONINFO osInfo;
> +  osInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> +  return GetVersionEx(&osInfo) && osInfo.dwMajorVersion >= 6;
> +}
> +#endif

Instead of duplicating this code just conditionally add nsWindowsHelpers.h when XP_WIN

@@ +2082,5 @@
>    if (sReplaceRequest) {
>      rv = ProcessReplaceRequest();
>    } else {
> +    if (sBackgroundUpdate) {
> +        LowerIOPriority();

I'm tempted to say we should only do this when updates are being applied without the user knowing (not from the about dialog and not from the update error wizard), but I don't think it'll make a big difference and it would take more effort than it is worth.

For example we could add an environment variable to specify when to do this, but that wouldn't work in Windows since the command line arguments are delegated to the service to do the update which doesn't retain the env.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73976ead8d6c

Comment 19

[(no longer active)]

Backed this out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e4bae8fe65

See bug 806028 on why.