Closed
Bug 517109
Opened 13 years ago
Closed 13 years ago
Fast Startup component should restart and issue memory-pressure notification periodically
Categories
(Toolkit Graveyard :: XULRunner, defect)
Tracking
(status1.9.2 beta5-fixed, fennec1.0a3-wm+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta5-fixed |
fennec | 1.0a3-wm+ | --- |
People
(Reporter: crowderbt, Assigned: crowderbt)
References
Details
Attachments
(2 files, 1 obsolete file)
4.36 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
140.17 KB,
image/png
|
Details |
+++ 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).
Assignee | ||
Comment 1•13 years ago
|
||
Comment on attachment 401137 [details] [diff] [review] v1 Gavin: Thoughts on this?
Attachment #401137 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 401137 [details] [diff] [review] v1 Shifting review.
Attachment #401137 -
Flags: review?(gavin.sharp) → review?(dolske)
Comment 4•13 years ago
|
||
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...
Attachment #401137 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Attachment #401137 -
Attachment is obsolete: true
Attachment #403335 -
Flags: review?(dolske)
![]() |
||
Comment 6•13 years ago
|
||
(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
Assignee | ||
Comment 7•13 years ago
|
||
> 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•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #403335 -
Flags: review?(dolske) → review+
Comment 9•13 years ago
|
||
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)
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
This can't be the cause, since this code isn't even packaged on OS X builds, and is WinCE only.
Assignee | ||
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
beltzner: Did this Ts regression go away as a result of either of my backouts?
Comment 14•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 18•13 years ago
|
||
Relanded: http://hg.mozilla.org/mozilla-central/rev/20d4d539beb1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b65e9fd158a
status1.9.2:
--- → beta1-fixed
Updated•13 years ago
|
Updated•6 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•