423132 - speed up sessionstore cookie bits

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[[e-mail]]

Attachment: proof-of-concept

we can drop 15-20ms from sessionstore's cookie saving code (run after every pageload) by doing things smarter. see bug 398807 comment 60, bug 398807 comment 61, and bug 398807 comment 62.

re-attaching the proof-of-concept patch in that bug, to get things rolling.

Comment 1

[Simon Bünzli]

Comment on attachment 309639 [details] [diff] [review]
proof-of-concept

Thanks for your efforts! I'd appreciate if you could tackle the SessionStore bits as well (otherwise I'd need a try-server build in order to having to adapt only nsSessionStore.js).

First, a somewhat unrelated, further speed-up could be achieved by splitting the following condition in two (first only the regex, then in a second step the !hosts and privacy level checks, so that we don't execute the second (file) regex if we've already seen the host):

>     function extractHosts(aEntry) {
>       if (/^https?:\/\/(?:[^@\/\s]+@)?([\w.-]+)/.test(aEntry.url) &&
>         !hosts[RegExp.$1] && _this._checkPrivacyLevel(_this._getURIFromString(aEntry.url).schemeIs("https"))) {

>+    aWindows.forEach(function(aWindow) {

This might be even faster as a classic for-loop (just not as elegant).

>+        for (var i = 0; i < aWindow._hosts.length; i++) {

Another few microseconds could be gained by starting at aWindow._hosts.length - 1 and counting down to 0.

>+          var list = cm.getCookiesFromHost(aWindow._hosts[i]);

This will be called several times if the same page is opened in several windows. What about caching the cookies you've already serialized and fall back on them if available?

Furthermore, this will yield some cookies twice if you have a page from e.g. groups.google.com and one from www.google.com open. Do cookies have unique IDs which you could remember and check before adding them?

Comment 2

[[e-mail]]

good suggestions - and in fact the lack of caching is probably why it wasn't working properly ;)

i won't be able to look at this for a while, but in the meantime, tryserver builds are available at:
https://build.mozilla.org/tryserver-builds/2008-03-19_17:[e-mail]-sscookie/

Comment 3

[Mike Schroepfer]

Simon want to take this? :-)

Comment 4

[[e-mail]]

Attachment: patch v1

okay, i had a couple hours spare tonight, and got things working. ;)

the patch is pretty self explanatory... i'm measuring sss_updateCookies times of around 2ms now. sss_restoreCookies is a bit slower (around 7ms for 5 tabs). i'm wondering, we push a jscookie for each tab it's used in, right - even if it's been pushed for another tab already? is that inefficient, or do we not actually store duplicate cookies? finding a way to make sss_restoreCookies faster would be good, at this point...

another random observation, _checkPrivacyLevel() looks up the pref each time. we could globally cache the pref w/ observer, might speed it up a bit...

Comment 5

[[e-mail]]

Comment on attachment 310723 [details] [diff] [review]
patch v1

builds at https://build.mozilla.org/tryserver-builds/2008-03-20_01:[e-mail]-sscookie2/

Comment 6

[Simon Bünzli]

(In reply to comment #4)
> we push a jscookie for each tab it's used in, right

Cookies are stored per window, which could indeed lead to duplicates being saved. This made it trivial to have get/setWindowState do the right thing WRT cookies and was a compromise opposed to storing cookies per tab. Do you have evidence that this has still a significant performance impact?

> finding a way to make sss_restoreCookies faster would be good

sss_restoreCookies is however by far not called as often as sss_updateCookies: usually only once per _restart_ (which should happen rarely enough). Anyway, the one point for potentially accelerating cookie restoring is by checking whether that specific cookie has already been set. Don't know whether it'd be worth it, though.

> _checkPrivacyLevel() looks up the pref each time.

AFAICT it used to be, but we were told that pref lookup was fast enough to be neglected. Feel free to change that, I'd be all for it.

>+      for (var host in aWindow._hosts) {

IIRC you're not guaranteed to have aWindow._hosts set at this point. Please check for it before using it (or do |... in aWindow._hosts || {}|).

Comment 7

[[e-mail]]

(In reply to comment #6)
> Cookies are stored per window, which could indeed lead to duplicates being
> saved. This made it trivial to have get/setWindowState do the right thing WRT
> cookies and was a compromise opposed to storing cookies per tab. Do you have
> evidence that this has still a significant performance impact?

not really - i think things are good enough with the current patch, was just curious.

> sss_restoreCookies is however by far not called as often as sss_updateCookies:
> usually only once per _restart_

yeah, still 7 - 10ms though - it's the only remaining thing we can speed up, really.

> the one point for potentially accelerating cookie restoring is by checking
> whether that specific cookie has already been set. Don't know whether it'd be
> worth it, though.

hmm, yeah. probably not. do you think it could be disk access time, or should the whole sessionstore.js file be parsed by then? i'll have a look if i get some time.

> > _checkPrivacyLevel() looks up the pref each time.
> AFAICT it used to be, but we were told that pref lookup was fast enough to be
> neglected. Feel free to change that, I'd be all for it.

i'll take a look.

> >+      for (var host in aWindow._hosts) {
> IIRC you're not guaranteed to have aWindow._hosts set at this point. Please
> check for it before using it (or do |... in aWindow._hosts || {}|).

i checked and it works as desired - if aWindow._hosts is null it doesn't enter the loop, or throw.

if the patch as it stands is okay with you, i think it's ready... i'll ask biesi to review the cookie bits; would you like to review the ss bits or do i need a firefox peer?

Comment 8

[Simon Bünzli]

(In reply to comment #7)
> hmm, yeah. probably not. do you think it could be disk access time

No, sessionstore.js is read once and then that data is used to restore windows, tabs, cookies, etc. Have you tried commenting out the line where the cookies are actually set (and counting how many (different) cookies are set during those 7-10 ms)?

> would you like to review the ss bits or do i need a firefox peer?

I've already reviewed most of it, so sure, I'll do the rest (you won't need review by another peer).

Comment 9

[[e-mail]]

Comment on attachment 310723 [details] [diff] [review]
patch v1

ok, sweet! hitting up biesi for r+sr (cookie bits only) and zeniko for sessionstore.

Comment 10

[Simon Bünzli]

Comment on attachment 310723 [details] [diff] [review]
patch v1

One further thing I noticed: We're now iterating over all windows three times in a row in a way where one loop would suffice:

>     // collect the cookies per window
>     for (var i = 0; i < aWindows.length; i++)
>       aWindows[i].cookies = [];

Just reset aWindow.cookies before you process all its hosts (keep the comment, though).

>+    var jscookies = [];

Nit: You're using this as a hash - so please make it an object: {}.

>+    var _this = this;

Nit: This isn't needed. Just add |this| as the second argument to aWindows.forEach and you can use the same |this| in the anonymous function.

>     // don't include empty cookie sections
>     for (i = 0; i < aWindows.length; i++)
>       if (aWindows[i].cookies.length == 0)
>         delete aWindows[i].cookies;

As above: Move the loop's body and the comment into the forEach loop.

r+ with the above changes and thanks again!

Comment 11

[[e-mail]]

Comment on attachment 310723 [details] [diff] [review]
patch v1

kicking to mconnor, since he's got some time for reviews at the moment.

Comment 12

[[e-mail]]

mconnor, review ping on this patch?

Comment 13

[[e-mail]]

Comment on attachment 310723 [details] [diff] [review]
patch v1

verbal sr- from mconnor with comments. new patch coming up sometime...

Comment 14

[Dietrich Ayala (:dietrich)]

what was the substance of mconnor's verbal r-? what else needs to be done here?

Comment 15

[[e-mail]]

creation time is now already exposed from bug 411952, so those bits can be removed from this patch. i believe mconnor also wanted a more granular API for getCookiesFromHost(), e.g. by adding additional fields to filter by - name, path, session/secure/httpOnly bits etc.

Comment 16

[[e-mail]]

Attachment: patch v2

updated to trunk. this patch doesn't rely on creationtime being unique anymore (hashes based on the host, path, name triplet). the getCookiesFromHost API remains the same - i don't think extending it to add searching by other fields is going to buy us much, since people can always get the whole cookie list and search arbitrarily on that. the point of this method is just to cut down the space significantly for one particular common case, searching by host. we can figure out an API for other search methods if you have some ideas though. (maybe have tristate args for isSession/isSecure etc, 0 = match false, 1 = match true, 2 = don't care? what about matching path and name?)

anyway, carrying over r=zeniko, since this is basically the same patch.

Comment 17

[[e-mail]]

we can use the getCookiesFromHost api in a followup to bug 460086, to speed up the fetching of cookies related to a given domain.

Comment 18

[Nochum Sossonko [:Natch]]

This patch will seemingly reduce Tp considerably, wanted?

Comment 19

[Dietrich Ayala (:dietrich)]

Dan, how risky is this change? Is this safe enough to take on a stability release for 3.5?

Comment 20

[[e-mail]]

pretty minor, so yes, i'd say it's safe enough to take - but we'd want to bake this on trunk for a couple weeks first, of course. it does modify an interface by adding a method, but that shouldn't present any compat issues.

Comment 21

[[e-mail]]

sdwilsh hath informed me that we're locked down on binary compat for 1.9.1, so we'd need to add a new interface for the additional method here. not a big deal, though.

Comment 22

[[e-mail]]

Comment on attachment 350310 [details] [diff] [review]
patch v2

kicking review to gavin.

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 350310 [details] [diff] [review]
patch v2

>+    aWindows.forEach(function(aWindow) {
>+      for (var host in aWindow._hosts) {
>+        var list = cm.getCookiesFromHost(host);
>+        while (list.hasMoreElements()) {
>+          var cookie = list.getNext().QueryInterface(Ci.nsICookie2);
>+          if (cookie.isSession && _this._checkPrivacyLevel(cookie.isSecure)) {
>+            // use the cookie's host, path, and name as a unique identifier,
>+            // to make sure we serialize each cookie only once
>+            var chost = cookie.host;
>+            var cpath = cookie.path;
>+            var cname = cookie.name;
>+            if (!jscookies[chost, cpath, cname]) {
>+              var jscookie = { host: chost, value: cookie.value };

This code, I do not think it does what you think it does.

js> var foo = [];
js> foo["a","b","c"] = "bar";
bar
js> foo["c"];
bar
js> foo["e","d","c"];
bar

Comment 24

[[e-mail]]

Attachment: patch v3

yes, indeed. fixed to use proper hashing, and adds unit test to verify all this actually works.

Comment 25

[Mike Connor [:mconnor]]

Comment on attachment 387756 [details] [diff] [review]
patch v3

sr=me on the API additions.

The /browser bits look better, but I can't SR and R the same patch.

Comment 26

[[e-mail]]

Comment on attachment 387756 [details] [diff] [review]
patch v3

gavin, do you have time to take a look? if not, i can ping dietrich...

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 387756 [details] [diff] [review]
patch v3

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
>--- a/browser/components/sessionstore/src/nsSessionStore.js
>+++ b/browser/components/sessionstore/src/nsSessionStore.js
>@@ -1502,75 +1502,95 @@ SessionStoreService.prototype = {
>   /**
>    * store all hosts for a URL
>    * @param aWindow
>    *        Window reference
>    */
>   _updateCookieHosts: function sss_updateCookieHosts(aWindow) {
>     var hosts = this._windows[aWindow.__SSi]._hosts = {};
>     
>-    // get all possible subdomain levels for a given URL
>+    // get the domain for each URL
>     var _this = this;

_this isn't used until the forEach below the extractHosts definition, so should be moved there.

actually, you can pass a "this" object to forEach, so you should just do that instead.

>+
>+    var jscookies = {};
>+    var _this = this;

ditto about passing to Array.forEach instead of using a local var.


>diff --git a/browser/components/sessionstore/test/browser/browser_423132.js b/browser/components/sessionstore/test/browser/browser_423132.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/sessionstore/test/browser/browser_423132.js
>@@ -0,0 +1,107 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is sessionstore test code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Daniel Witte <[e-mail]>.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.

2009, or has this been around since then?

>@@ -74,15 +74,24 @@ interface nsICookie2 : nsICookie
>     readonly attribute PRInt64 expiry;
> 
>     /**
>      * true if the cookie is an http only cookie
>      */
>     readonly attribute boolean isHttpOnly;
> 
>     /**
>-     * the creation time of the cookie, in microseconds
>+     * the creation time of the cookie, in microseconds (not seconds!)
>      * since midnight (00:00:00), January 1, 1970 UTC.
>      */
>     readonly attribute PRInt64 creationTime;
> 
>+    /**
>+     * the last time the cookie was accessed (i.e. created,
>+     * modified, or read by the server), in microseconds
>+     * (not seconds!) since midnight (00:00:00), January 1,
>+     * 1970 UTC.
>+     *
>+     * note that this time may be approximate.
>+     */
>+    readonly attribute PRInt64 lastAccessed;
> 
> };

maybe it's just me, but "(not seconds!)" seems superfluous, since the word "microseconds" makes itself pretty clear.

nit: uppercase first word in sentence.

>   /**
>+   * Returns an enumerator of cookies that would be returned to a given host,
>+   * ignoring the cookie flags isDomain, isSecure, and isHttpOnly. Thus, for a
>+   * host "weather.yahoo.com", host or domain cookies for "weather.yahoo.com"
>+   * and "yahoo.com" would be returned, while a cookie for "my.weather.yahoo.com"
>+   * would not.
>+   *
>+   * @param aHost
>+   *        the host string to look for, e.g. "google.com". this should consist
>+   *        of only the host portion of a URI, and should not contain a leading
>+   *        dot, a port, etc.

ditto the previous nit

Comment 28

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/ace719621c56

Thanks... fixed!

Comment 29

[Boris Zbarsky [:bzbarsky]]

Could this have caused the 3.23% Dromaeo regression pointed to here: http://groups.google.com/group/mozilla.dev.tree-management/msg/7344f39035b7a9f7 ?

Comment 30

[Jim Jeffery not reading bug-mail 1/2/11]

I think this has also broken Private Browsing
Seems that Trying to enter Private Browsing is broken..I get a blank window with un-named tab but no PB, trying to start PB a second time it starts up. The problem is that the sessioin is lost when you exit PB. Session can be restored with History -> Restore last session, but not sure how long that would work before trashing the whole session.

2 errors in the console:
Error: Exception thrown while processing the private browsing mode change request: [Exception... "'[JavaScript Error: "this._getURIFromString is not a function" {file: "file:///J:/Program%20Files%20(x86)/Minefield/components/nsSessionStore.js" line: 1513}]' when calling method: [nsISessionStore::getBrowserState]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: file:///J:/Program%20Files%20(x86)/Minefield/components/nsPrivateBrowsingService.js :: PBS__onBeforePrivateBrowsingModeChange :: line 136" data: yes]
Source file: file:///J:/Program%20Files%20(x86)/Minefield/components/nsPrivateBrowsingService.js
Line: 333

Error: this._getURIFromString is not a function
Source file: file:///J:/Program%20Files%20(x86)/Minefield/components/nsSessionStore.js
Line: 1513

Looks like once you hit: Start Private Browsing you get the Dialog box pop-up
Then clicking Start PB ... it does not start leaving you with the present session, but the errors from sessionstore.js continue to fill up the Console2

EDIT: works with new Profile, renaming sessionstore.js did not help...
EDIT: Renaming places.sqlite did not help
EDIT: Correction, a new profile does NOT fix this... just deleted all my profiles, created a new one, and ta-da, does not work, no addons had been installed yet.

Comment 31

[Jim Jeffery not reading bug-mail 1/2/11]

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=504600

Comment 32

[[e-mail]]

Thanks, looks like the scoping didn't work when moving _this into the forEach. Will fix shortly.

Comment 33

[[e-mail]]

Fix checked in. Please comment here or in bug 504600 if you still see issues.

Comment 34

[[e-mail]]

(In reply to comment #29)
> Could this have caused the 3.23% Dromaeo regression pointed to here:
> http://groups.google.com/group/mozilla.dev.tree-management/msg/7344f39035b7a9f7
> ?

This should have no effect on JS engine speed, which I believe is all the Dromaeo test tickles.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Dromaeo touches large parts of the DOM, not just JS.

There are at least two callsites where it touches document.cookie: one read and one write.  I don't know whether those are relevant to the numbers it measures.

Comment 36

[[e-mail]]

Hmm, ok. The impact of this patch, if any, would be on performance when opening new windows/tabs. (And then, not as part of pageload, but later when the sessionstore timer fires.) So if the test is thrashing windows and/or tabs, then it's definitely worth looking into.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Would this test kick in when creating iframes too?

Comment 38

[[e-mail]]

Yep, apparently it will... does the test create enormous amounts of iframes? (If this patch regressed window/tab opening significantly, we should be seeing blips in other tests as well, so the most logically consistent explanation for an increase would require the 'enormous' qualifier.)

Comment 39

[Boris Zbarsky [:bzbarsky]]

Dromaeo runs each test in a new iframe, iirc.

Comment 40

[Eric Shepherd [:sheppy]]

The documentation has been updated; in addition, I added information about previously existing methods that were not yet documented.

https://developer.mozilla.org/en/nsICookieManager2
https://developer.mozilla.org/en/nsICookie2