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

RESOLVED WONTFIX

Status

()

Toolkit
Application Update
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla19
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 670521 [details] [diff] [review]
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.
Attachment #670521 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 1

6 years ago
This should actually probably use IOPOL_SCOPE_PROCESS instead of IOPOL_SCOPE_THREAD
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
You can r? me with the changes by the way.
(Assignee)

Comment 4

6 years ago
Created attachment 670891 [details] [diff] [review]
Use low priority IO on OS X V2
Attachment #670521 - Attachment is obsolete: true
Attachment #670521 - Flags: review?(robert.bugzilla)
Attachment #670891 - Flags: review?(netzen)

Comment 5

6 years ago
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.
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 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.
Attachment #670891 - Flags: review?(netzen) → review-
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

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

Comment 10

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

Comment 12

6 years ago
Created attachment 670921 [details] [diff] [review]
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.
Attachment #670891 - Attachment is obsolete: true
Attachment #670921 - Flags: review?(netzen)
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.
Attachment #670921 - Flags: review?(netzen) → review-
Also, you attached the wrong patch for review by the way.
(Assignee)

Comment 15

6 years ago
Created attachment 670950 [details] [diff] [review]
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.
Attachment #670921 - Attachment is obsolete: true
Attachment #670950 - Flags: review?(netzen)

Comment 16

6 years ago
Fine!
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.
Attachment #670950 - Flags: review?(netzen)
https://hg.mozilla.org/mozilla-central/rev/73976ead8d6c
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Summary: Use low priority IO on OS X → Use low priority IO on Windows and OS X
Summary: Use low priority IO on Windows and OS X → Use low priority IO for staged updates in Windows and OS X
Depends on: 806028

Comment 19

6 years ago
Backed this out:

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

See bug 806028 on why.
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.