506482 - Don't write sessionstore.js to disk for "read only" events

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Damon Hsu-Hung]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1

Although I appreciate that session restore really does restore every detail of my session, this becomes unwieldy when sessionstore.js gets large. With just 61 tabs across 3 windows, my sessionstore.js is nearly 2MB. This results in over half a gigabyte of disk writes after an hour of browsing!

This 2MB file is rewritten in its entirety whenever:

1. I open or close a tab (obviously)
2. I click a link (since session restore remembers every tab's history)
3. I switch to another tab (since it remembers the currently open tab)
4. I scroll within a tab (since it remembers where I am in the page!)

Therefore, despite the 10-second hysteresis in rewriting this file, an active browsing session means that it probably will get rewritten every 10 seconds. At 2MB a pop, that's 720MB per hour.

On my older machines, this is a very noticeable performace hit, especially when opening new tabs; on my laptops, this wastes power; on my SSDs, it wastes flash cycles.

Reproducible: Always

Steps to Reproduce:
1. Open many tabs until sessionstore.js gets quite large. (I am not sure why some pages grow sessionstore.js by a few kilobytes and other pages grow it by hundreds. As a rule of thumb, opening every article on the front page of CNN.com will get you close to 2MB.)

2. Open task manager (or equivalent) and watch the total disk writes generated by the Firefox process. Or use procmon (or equivalent) and watch Firefox rewrite sessionstore.js.

3. Observe rewrites occurring whenever a tab is opened, a link is clicked, a tab is selected, or a page is scrolled. Observe Firefox write multiple gigabytes after a mere evening's worth of ordinary browsing.
Actual Results:  
For my usage patterns, I see Firefox writing 2-3GB per day, sometimes more. The vast majority of this is sessionstore activity, with sqllite contributing comparatively little.

Expected Results:  
The session should be stored in a way that's amenable to partial updates. Throwing it into sqllite is probably overkill. Would it be difficult to write session activity as a log, and rewrite it only occasionally?

Some of the updates are extremely small.  Here are the diffs I get when I click on a new tab:

< {"state":"running","lastUpdate":1248521427588}})
--
> {"state":"running","lastUpdate":1248521292034}})

And here are the diffs I get when I scroll within an existing tab:

< {"state":"running","lastUpdate":1248521486237}})
--
> {"state":"running","lastUpdate":1248521427588}})

So in the worst-case scenario, where I'm not opening any tabs but simply *reading for an hour*, I could conceivably generate 720MB of disk writes to update as little as 1800 bytes of data (or 1.4MB, using 4k clusters). And all that just to remember where I've scrolled to in the page.

At the least, please add an option to tweak the behavior.

This is a fresh install of Firefox 3.5.1 on a fresh install of XP SP3 with the latest patches. The problem existed before I installed any extensions.

Comment 1

[Jo Hermans]

http://kb.mozillazine.org/Browser.sessionstore.interval

The default is 10 seconds, but can be changed.

Comment 2

[Michael Kohler [:mkohler]]

Like Jo said, the default is 10 seconds. AFAIK the sessionrestore file is not rewritten more often. Please check your config entry. IMO this bug is either WORKSFORME or INVALID.

Another possibility would be that an addon causes this heavy file-rewriting, but this is unlikely, I guess.

Comment 3

[Damon Hsu-Hung]

@Jo, thanks for the pointer to the config variable. I searched Bugzilla but didn't realize there's a KB.

@Michael, as I stated, rewriting a 2MB file every 10 seconds is *already* enough to generate gigabytes of traffic (720MB/hour). Also, note that I had no extensions loaded when I first observed this issue.

IMHO, this rewriting behavior still merits some consideration even if it is working as designed. Perhaps this should be marked as an enhancement, but it seems to me that it shouldn't be so easy to make Firefox generate this much disk traffic. Yet, the test case I've provided is certainly not farfetched (indeed, I'd say it resembles typical Googling behavior).

Even after wiping sessionstore.js clean, I *always* find it approaching 2MB after just an hour of heavy activity, or a day of casual activity. If this is true for the average user, then at least it raises the question of whether the default interval is too short.

But even if the interval is lengthened, it nevertheless seems to me that rewriting a 2MB file to change 5 bytes is a phenomenon worth avoiding.

Comment 4

[Michael Kohler [:mkohler]]

(In reply to comment #3)
> Even after wiping sessionstore.js clean, I *always* find it approaching 2MB
> after just an hour of heavy activity, or a day of casual activity. If this is
> true for the average user, then at least it raises the question of whether the
> default interval is too short.

Right now you just lose 10 seconds of work when Firefox crashes. Higher the interval would mean to lose more. I don't think this is what an average user wants.

> But even if the interval is lengthened, it nevertheless seems to me that
> rewriting a 2MB file to change 5 bytes is a phenomenon worth avoiding.

Indeed.

Comment 5

[Jo Hermans]

The file is only written when something changes. This doesn't only happen when clicking a link, but also when typing in data in a form (so that the contents can be restored), or scrolling (to save position). Unfortunately, taking those features away would make people angry.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I filed bug 508740 for converting to some sort of database. That would allow us to mark dirty states and only update the data for the changed items (eg. on a per tab basis). So this bug will likely never see any specific fix, but will get fixed when that other bug does.

Comment 7

[Thomas de Grenier de Latour]

(In reply to comment #5)
> The file is only written when something changes. This doesn't only happen when
> clicking a link, but also when typing in data in a form (so that the contents
> can be restored), or scrolling (to save position). Unfortunately, taking those
> features away would make people angry.

As far as I'm concerned, I couldn't care less about losing my page scrolling level.  But I do care about this recurring disk activity when simply reading some already opened pages.  I propose that you add the option of not triggering the sessionstore just for "read-only" events (scrolling a page or selecting a tab). A patch follows (defaults to the current behavior).

Comment 8

[Thomas de Grenier de Latour]

Attachment: sessionstore_ignore_readonly_events.patch

Add the option to not trigger sessionstore for read only events (tab selection or page scrolling).
Patch made against FF 3.6_beta4 sources.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 417359 [details] [diff] [review]
sessionstore_ignore_readonly_events.patch

dietrich, mconnor & I talked about this and think this is a good bandaid, but we'll need to do something more (probably bug 508740) for the long term. So we're going to go ahead with this, get it on trunk and hopefully into a 3.6.1 release (we won't block 3.6 on it).

>+  // whether read-only events (tab selection and page scrolling) should be ignored
>+  _ignore_readonly_events: false,
>+
>   // when crash recovery is disabled, session data is not written to disk
>   _resume_from_crash: true,

I know _resume_from_crash isn't camel case, but don't mimic that. Please use camel case (here, but not in pref name).

I can't give this r+ without tests. Please write a test file making sure this actually works.

(In reply to comment #8)
> Patch made against FF 3.6_beta4 sources.

Typically we patch against mozilla-central and backport to branch. It shouldn't make much difference (if any, except line numbers) here, so if you could do that as well, that would be great.

Comment 10

[Justin Dolske [:Dolske]]

I think it was also suggested to drop the pref from the patch, and just make this the normal behavior.

Comment 11

[Thomas de Grenier de Latour]

Attachment: ignore_readonly_events-v1.1.patch

Thanks all for the review and comments.

Here is v1.1 of the pach, with the following changes:
 - patch made against mozilla-central (and also tested on the 1.9.2 branch)
 - variable "_ignore_readonly_events" renamed into "_ignoreReadOnlyEvents"
 - added a mochitest (first check that, with the default option value, tab
 select and scroll events are still triggering a sessionstore, and then that,
 once the option is changed, it doesn't anymore)

Some more notes:
 - the mochitest uses several timers, to wait for sessionstore.js updates, and 
 thus takes ~22 seconds to run (sessionstore has some 3 seconds delay in some
 cases, so i've choosed 5 seconds to be safe).  I hope that's ok (I've found no
 policy about acceptable test run time), otherwise let me know (i might need 
 some hints for an alternative test strategy).
  - Justin, about making this a mandatory behavior: i've left the option
 as-is for now, just let me now if a different decision is taken.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 417834 [details] [diff] [review]
ignore_readonly_events-v1.1.patch

My mistake - we're actually just going to NOT save at all for readonly events. So you actually get to remove code instead of adding it. Sorry for the confusion.

This should help make your test a little bit simpler as well.

>diff -r ea06f16ef9e7 browser/components/sessionstore/test/browser/browser_506482.js

Please include the license here (and make sure your name goes in it).

>+function test() {
>+  /** Test for Bug 506482 **/
>+
>+  // test setup
>+  waitForExplicitFinish();
>+
>+  let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+           getService(Ci.nsISessionStore);
>+
>+  // read the sessionstore.js mtime (picked from browser_248970_a.js)
>+  let profilePath = Cc["@mozilla.org/file/directory_service;1"].
>+                    getService(Ci.nsIProperties).
>+                    get("ProfD", Ci.nsIFile);
>+  function getSessionstoreFile() {
>+    let sessionStoreJS = profilePath.clone();
>+    sessionStoreJS.append("sessionstore.js");
>+    return sessionStoreJS;
>+  }
>+  function getSessionstorejsModificationTime() {
>+    let file = getSessionstoreFile();
>+    if (file.exists())
>+      return file.lastModifiedTime;
>+    else
>+      return -1;
>+  }
>+
>+  // make sure sessionstore.js is saved ASAP on all events
>+  gPrefService.setIntPref("browser.sessionstore.interval", 0);
>+  gPrefService.setBoolPref("browser.sessionstore.ignore_readonly_events", false);
>+
>+  // create and select a first tab
>+  let tab1 = gBrowser.addTab();

You can actually call addTab with a URI.

>+  gBrowser.selectedTab = tab1;
>+  tab1.linkedBrowser.addEventListener("load", function loadListener1(e) {
>+    tab1.linkedBrowser.removeEventListener("load", arguments.callee, false);
>+
>+    // create one more tab
>+    let tab2 = gBrowser.addTab();
>+    gBrowser.selectedTab = tab2;
>+    tab2.linkedBrowser.addEventListener("load", function loadListener2(e) {
>+      tab2.linkedBrowser.removeEventListener("load", arguments.callee, false);
>+
>+      // sleep and get initial sessionstore.js mtime
>+      setTimeout(function step1(e) {
>+        var mtime1 = getSessionstorejsModificationTime();

Nit: Use let.

>+        ok(mtime1 > -1, "initial sessionstore.js update");
>+
>+        // test sessionstore.js is still updated on tab selection
>+        gBrowser.selectedTab = tab1;
>+        setTimeout(function step2(e) {
>+          var mtime2 = getSessionstorejsModificationTime();
>+          ok(mtime2 > mtime1, "tab selection: sessionstore.js updated");
>+
>+          // test sessionstore.js is still updated on scrolling
>+          tab1.linkedBrowser.contentWindow.scrollTo(1100, 1200);
>+          setTimeout(function step3(e) {
>+            var mtime3 = getSessionstorejsModificationTime();
>+            ok(mtime3 > mtime2, "content scrolling: sessionstore.js updated");
>+
>+            // now redo tests with ignore_readonly_events=true (because changing
>+            // the pref happens to update the sessionstore.js, sleep again)
>+            gPrefService.setBoolPref("browser.sessionstore.ignore_readonly_events", true);
>+            setTimeout(function step4(e) {
>+              var mtime4 = getSessionstorejsModificationTime();
>+
>+              // test sessionstore.js is no more updated on tab selection or scrolling
>+              gBrowser.selectedTab = tab2;
>+              tab2.linkedBrowser.contentWindow.scrollTo(1100, 1200);
>+              setTimeout(function step5(e) {
>+                var mtime5 = getSessionstorejsModificationTime();
>+                ok(mtime5 == mtime4, "tab selection and content scrolling: sessionstore.js not updated");

Try to keep lines no longer than 80 characters. Some exceptions are ok. For example, here, breaking it up such that the mtime5 and "tab..." are lined up is ok.

Also for equality comparisons, use the is(val, expected, "") method

>+
>+                // ok, done, cleanup and finish
>+                if (gPrefService.prefHasUserValue("browser.sessionstore.interval"))
>+                  gPrefService.clearUserPref("browser.sessionstore.interval");
>+                if (gPrefService.prefHasUserValue("browser.sessionstore.ignore_readonly_events"))
>+                  gPrefService.clearUserPref("browser.sessionstore.ignore_readonly_events");
>+                gBrowser.removeTab(tab1);
>+                gBrowser.removeTab(tab2);
>+                finish();
>+              }, 5000);
>+            }, 5000);
>+          }, 5000);
>+        }, 5000);
>+      }, 2000);
>+    }, true);
>+    tab2.linkedBrowser.loadURI("data:text/html,<body style='width: 100000px; height: 100000px;'><p>top</p></body>");
>+  }, true);
>+  tab1.linkedBrowser.loadURI("data:text/html,<body style='width: 100000px; height: 100000px;'><p>top</p></body>");
>+}

(In reply to comment #11)
>  - the mochitest uses several timers, to wait for sessionstore.js updates, and 
>  thus takes ~22 seconds to run (sessionstore has some 3 seconds delay in some
>  cases, so i've choosed 5 seconds to be safe).  I hope that's ok (I've found no
>  policy about acceptable test run time), otherwise let me know (i might need 
>  some hints for an alternative test strategy).

It should be OK. You shouldn't have to worry as much about that now. You should be able to do the actions that don't trigger a save then do one that performs an immediate save (ie. calls saveState instead of saveStateDelayed) with appropriate checks. I think you might be ok doing it serially, but if not, executeSoon should be good enough (and preferred over setTimeout when possible)

Next version I can upload to try server and see how it fares there. That testing environment is much closer to our automated system, and often tests with delicate timing don't fare as well there.

Comment 13

[Thomas de Grenier de Latour]

Attachment: ignore_readonly_events-v1.2.patch

Hi,

Thanks for this second review.

Here is v1.2, with the following changes:
 - instead of adding an option, avoid sessionstore on tab select and scroll events.  The "scroll" event listener is completely removed since there was nothing left to do on it.
 - as a consequence, simplified the test case: it now simply open some tabs, wait for a first sessionstore, change selected tab, scroll, wait a bit more, and check sessionstore.js has not been updated.
 - taken your Javascript advices into account (80 chars lines, s/var/let/, URL parameter to addTab).

About the first timer (the one waiting for a reference sessionstore.js mtime), i couldn't avoid it, simply because adding the initial tabs creation triggers a saveStateDelayed(), which I can't cancel (even forcing an immediate saveState() by changing the browse.sessionstore.interval preference lets the background _saveTimer running).  A solution would be to cancel _saveTimer (if any) in the saveStore() function: i think it would be a good idea, but out of scope for this patch.

Also note that i've successfully run the full sessionstore tests suite, but i had to launch some CPU hogs in the background, otherwise it hangs.  I've verified it was not related to my changes, and have found a similar bug reports (bug #518970).

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 418025 [details] [diff] [review]
ignore_readonly_events-v1.2.patch

Sorry for not getting back to you sooner. This slipped my mind.

It's looks really good. Just a few nits and we should be good to go.

>diff -r ea06f16ef9e7 browser/components/sessionstore/test/browser/browser_506482.js
>+  // create and select a first tab
>+  let tab1 = gBrowser.addTab(TEST_URL1);
>+  tab1.linkedBrowser.addEventListener("load", function loadListener1(e) {
>+    tab1.linkedBrowser.removeEventListener("load", arguments.callee, false);

You're attaching these event listeners with useCapture=true, so you need to remove them with useCapture=true.

>+    gBrowser.selectedTab = tab1;

This doesn't add anything to the test, does it?

>+
>+    // create one more tab
>+    let tab2 = gBrowser.addTab(TEST_URL2);
>+    tab2.linkedBrowser.addEventListener("load", function loadListener2(e) {
>+      tab2.linkedBrowser.removeEventListener("load", arguments.callee, false);
>+
>+      // step1: the above has triggered some saveStateDelayed(), sleep until
>+      // it's done, and get the initial sessionstore.js mtime
>+      setTimeout(function step1(e) {
>+        let mtime1 = getSessionstorejsModificationTime();
>+        isnot(mtime1, -1, "initial sessionstore.js update");
>+
>+        // step2: test sessionstore.js is not updated on tab selection
>+        // or content scrolling
>+        gBrowser.selectedTab = tab2;
>+        tab2.linkedBrowser.contentWindow.scrollTo(1100, 1200);
>+        setTimeout(function step2(e) {
>+          let mtime2 = getSessionstorejsModificationTime();
>+          is(mtime2, mtime1,
>+             "tab selection and scrolling: sessionstore.js not updated");
>+
>+          // ok, done, cleanup and finish
>+          if (gPrefService.prefHasUserValue(PREF_INTERVAL))
>+            gPrefService.clearUserPref(PREF_INTERVAL);
>+          gBrowser.removeTab(tab1);
>+          gBrowser.removeTab(tab2);
>+          finish();
>+        }, 5000); // end of sleep after tab selection and scrolling
>+      }, 5000); // end of sleep after initial saveStateDelayed()

I still don't like having tests depend on setTimeout, but it's probably the best way to do this. The other way involves observing file written notifications.

Also, there is an execution timeout for tests. This shouldn't hit it, but lets try a timeout of 2500. I would expect the file to be written within that time even for saveStateDelayed.

>+    }, true);
>+  }, true);

(here's your userCapture=true)

>+}

Comment 15

[Thomas de Grenier de Latour]

Attachment: ignore_readonly_events-v1.3.patch

Hi Paul,

Thanks for this review, and sorry for the late reply, I was in vacations.

Here is v1.3, taking your comments into account:
 - simplify the test case a bit, by removing one of the tabs (was not really necessary)
 - make coherent use of userCapture=true
 - reduce delays to 2.5 secs.  I've run repeated tests to check that it was enough (also on a very loaded system).  I've also checked that the test case fails if I drop my changes to nsSessionStore.js  (which would not have been so if the 2nd timer was not long enough).

Comment 16

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #15)
> Here is v1.3, taking your comments into account:

Thanks again. I uploaded to the try server just to make sure the tests go fine here. If it looks good, I'll check it in (hopefully today).

>  - reduce delays to 2.5 secs.  I've run repeated tests to check that it was
> enough (also on a very loaded system).  I've also checked that the test case
> fails if I drop my changes to nsSessionStore.js  (which would not have been so
> if the 2nd timer was not long enough).

Ah true, I forgot that onTabScroll used 3s in saveStateDelayed and so the 2nd timer would need to be longer to properly test the old case. That should be fine though. We still test the current condition.

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 420150 [details] [diff] [review]
ignore_readonly_events-v1.3.patch

r=zpao. Thanks for taking care of this Thomas.

Comment 18

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/36bd078ee596

Comment 19

[Thomas de Grenier de Latour]

Thanks Paul for your help in turning this into an acceptable patch.  Your reviews and advices have been quite instructive.

Comment 20

[u279076]

What can QA do to verify this bug?

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #20)
> What can QA do to verify this bug?

Verify that sessionstore.js doesn't change after scrolling or switching tabs. Ensure it saves once first (e.g. do something else, wait 15s or for next write), then do the "read only" events.

Comment 22

[u279076]

Verified fixed.