517109 - Fast Startup component should restart and issue memory-pressure notification periodically

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Brian Crowder]

Attachment: v1

+++ This bug was initially created as a clone of Bug #509249 +++

I'm not sure what a good interval is for this right now, so I'm going w/ 30 seconds after last-window-close for memory-pressure, and 15 minutes after that for silent restart.  It's possible or even likely that something much less frequent is better, or that we should be basing the heuristic on phone usage itself (ie., only do the expensive/costly browser restart if the phone is long-idle).

Comment 1

[Brian Crowder]

Comment on attachment 401137 [details] [diff] [review]
v1

Gavin:  Thoughts on this?

Comment 3

[Brian Crowder]

Comment on attachment 401137 [details] [diff] [review]
v1

Shifting review.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 401137 [details] [diff] [review]
v1

>+const fsRestartEnv = "FASTSTART_INITIATED_RESTART";

Make this caps, RESTART_ENV_VAR or something?

>+function setenv(key, value) {
>+  let env = Cc["@mozilla.org/process/environment;1"].
>+            getService(Components.interfaces.nsIEnvironment);
>+  env.set(key, value);
>+}

I assume the environment bits are working on WinCE/WinMo? ISTR Rob Strong had issues with that when working on the updater...

>+  let _shutdown;

better name: _isShuttingDown (= false).

>       // XXX start our long (15 min?) stub restart timer here

This comment can go away now.

>+      // restart the stub in 15 minutes
>+      _restartTimer = new Timer(restart, 60000 * 15, Ci.nsITimer.TYPE_ONE_SHOT);

I think it would be a bit cleaner to start both timers at the same time. Basically, saying the last window closed, so schedule 2 timers to fire (+30s, +15m). Helps disentagle what's going on, with not waiting for one to fire before starting the 2nd.

And null out _restartTimer in restart(), just to be sure.

>   handle: function fs_handle(cmdLine) {
>     // the rest of this only handles -faststart here
>-    if (cmdLine.handleFlag("faststart-hidden", false))
>+    if (cmdLine.handleFlag("faststart-hidden", false) || (getenv(fsRestartEnv) == "1"))
>       cmdLine.preventDefault = true;

Also call stopMemoryCleanup() here?

There's a period of time between the browser being requested to open a window, and when domwindowopened fires. If the restart timer happens to fire then, it would be rather unfortunate. Kind of edgecasy, but I'd prefer to be paranoid about not quitting in the middle of the user doing something. :)

r+ with these fixes...

Comment 5

[Brian Crowder]

Attachment: another spin

(In reply to comment #4)
> I assume the environment bits are working on WinCE/WinMo? ISTR Rob Strong had
> issues with that when working on the updater...

Yes, the shunt preserves environment variables and then passes them to the child process on restart as part of the command-line, so Firefox -> Firefox or Fennec -> Fennec invocations should be okay.  Not sure what problem the updater encounters here.  I'll double check this on WinMo w/ Fennec before I land.

> better name: _isShuttingDown (= false).

The "= false" isn't necessary since undefined will evaluate as false in boolean expressions, but I can add it if you insist.


> >   handle: function fs_handle(cmdLine) {
> >     // the rest of this only handles -faststart here
> >-    if (cmdLine.handleFlag("faststart-hidden", false))
> >+    if (cmdLine.handleFlag("faststart-hidden", false) || (getenv(fsRestartEnv) == "1"))
> >       cmdLine.preventDefault = true;
> 
> Also call stopMemoryCleanup() here?
> 
> There's a period of time between the browser being requested to open a window,
> and when domwindowopened fires. If the restart timer happens to fire then, it
> would be rather unfortunate. Kind of edgecasy, but I'd prefer to be paranoid
> about not quitting in the middle of the user doing something. :)
> 
> r+ with these fixes...

I'm not sure I understand what edge-case you're concerned about here.

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
> Created an attachment (id=403335) [details]
> another spin
> 
> (In reply to comment #4)
> > I assume the environment bits are working on WinCE/WinMo? ISTR Rob Strong had
> > issues with that when working on the updater...
> 
> Yes, the shunt preserves environment variables and then passes them to the
> child process on restart as part of the command-line, so Firefox -> Firefox or
> Fennec -> Fennec invocations should be okay.  Not sure what problem the updater
> encounters here.  I'll double check this on WinMo w/ Fennec before I land.
This was in regards to bug 510836 where the updater was changed to use the shunt's environment.cpp

Comment 7

[Brian Crowder]

> This was in regards to bug 510836 where the updater was changed to use the
> shunt's environment.cpp

So this works correctly for you?  Great!

Comment 8

[Justin Dolske [:Dolske]]

(In reply to comment #5)

> I'm not sure I understand what edge-case you're concerned about here.

The sequence I'm thinking of is:

1) close last window
2) restart gets scheduled... time passes...
3) external app asks browser to open a new URL
4) existing browser process gets request to open new URL
5) restart timer fires before we get around to firing domwindowopened.

I guess you'd actually need to fix this elsewhere, since the existing browser process wouldn't have it's command line handler called again at step 4 (afaik?). So, eh, maybe worth a followup bug.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

This possibly caused a Ts regression (on dirty profiles, OSX Leopard):

http://graphs.mozilla.org/graph.html#tests=[{%22machine%22:169,%22test%22:52,%22branch%22:1},{%22machine%22:170,%22test%22:52,%22branch%22:1},{%22machine%22:171,%22test%22:52,%22branch%22:1},{%22machine%22:172,%22test%22:52,%22branch%22:1},{%22machine%22:173,%22test%22:52,%22branch%22:1},{%22machine%22:174,%22test%22:52,%22branch%22:1},{%22machine%22:175,%22test%22:52,%22branch%22:1},{%22machine%22:176,%22test%22:52,%22branch%22:1},{%22machine%22:177,%22test%22:52,%22branch%22:1},{%22machine%22:178,%22test%22:52,%22branch%22:1},{%22machine%22:179,%22test%22:52,%22branch%22:1},{%22machine%22:180,%22test%22:52,%22branch%22:1},{%22machine%22:181,%22test%22:52,%22branch%22:1},{%22machine%22:182,%22test%22:52,%22branch%22:1}]&sel=1255984020,1256156820

Previous results: 995.474 from build 20091020115342 of revision 0facea043ada at 2009-10-20 12:46:00 on talos-rev2-leopard03 run # 0

New results: 1029.63 from build 20091020124334 of revision 9fa60d07b156 at 2009-10-20 13:27:00 on talos-rev2-leopard08 run # 0

(the other patch in the checkin range, bug 519843, is WinCE only)

Comment 10

[Brian Crowder]

This component isn't used on Mac OS X at all, unless we've added --enable-faststart to our mozconfigs for those platforms?  I'd be happy to backout, if we still believe this is the cause.

Comment 11

[Justin Dolske [:Dolske]]

This can't be the cause, since this code isn't even packaged on OS X builds, and is WinCE only.

Comment 12

[Brian Crowder]

Dolske, that's what I said too, but I backed it out anyway.  Will try to reland today unless Ts mystically fixed itself after my backout.

Comment 13

[Brian Crowder]

beltzner:  Did this Ts regression go away as a result of either of my backouts?

Comment 14

[Justin Dolske [:Dolske]]

The graph doesn't show any difference, so it wasn't this. Go ahead and reland it. [TBH, the graph is noisy enough that this might not even be a real regression.]

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

The graph does, in fact, go down on the 22nd. I'm not sure I agree with you, Dolske. Reload the link from comment 9 and expand the time range.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Here's a graphic of the range: http://grab.by/bxo

I guess check it back in and see if it goes up again? I guess it could be poorly timed noise, but that looks like an obvious bump to me.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Annotated perf graph

The two changesets in question:

Landed:  http://hg.mozilla.org/mozilla-central/rev/899023a9fbb0
Backout: http://hg.mozilla.org/mozilla-central/rev/45a3ded46ebc

This screenshot hilights the duration the patch was in the tree. The apparent increase in Ts begins immediately after the landing time, but persists for about a day after the backout.

The reason I say this might not even be a real regression is that not all of the Talos boxes agree there was a regression. Some went up, some didn't. At least one box went down before the backout, and then back up after the backout. Also, if you look at the period from around October 1st October 10th, there are a number of other instances where a couple of boxes spiked and then dropped, while other boxes did not.

Comment 18

[Brian Crowder]

Relanded:
http://hg.mozilla.org/mozilla-central/rev/20d4d539beb1

Comment 19

[Brian Crowder]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b65e9fd158a