524371 - Port Bug 423132 [speed up sessionstore cookie bits] to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Misak Khachatryan]

From parent bug:

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.

Comment 1

[Misak Khachatryan]

Attachment: patch with test

Comment 2

[[e-mail]]

Comment on attachment 425791 [details] [diff] [review]
patch with test

>+    // get the domain for each URL
s/domain/host/, surely?

>-      if (/^https?:\/\/(?:[^@\/\s]+@)?([\w.-]+)/.test(aEntry.url) &&
>-        !hosts[RegExp.$1] && _this._checkPrivacyLevel(_this._getURIFromString(aEntry.url).schemeIs("https"))) {
>+      if (/^https?:\/\/(?:[^@\/\s]+@)?([\w.-]+)/.test(aEntry.url)) {
>+        if (!hosts[RegExp.$1] && _this._checkPrivacyLevel(_this._getURIFromString(aEntry.url).schemeIs("https"))) {
Not sure why this was changed from an && to a nested if?
(If you still want to change it, I would also change the _this._getURIFromString(aEntry.url).schemeIs("https") to
aEntry.url.substr(0, 5) == "https" to simplify the code)
(or if you're really sneaky use extra capturing brackets
to grab the https into RegExp.$1 and test that instead.)

>+      if (!aHash[aHost][aPath][aName])
>+        aHash[aHost][aPath][aName] = {};
>+
>+      aHash[aHost][aPath][aName] = aCookie;
Nit: Setting aHash[aHost][aPath][aName] twice; the first time is unnecessary.

>+            // use the cookie's host, path, and name as keys into a hash,
>+            // to make sure we serialize each cookie only once
>+            var isInHash = false;
>+            try {
>+              if (jscookies[cookie.host][cookie.path][cookie.name])
This showed up as a JS strict warning when I tested (presumably because I was using a debug build which has it on by default?) Rather than using try/catch you should use part of addCookieToHash here instead to ensure that the parts of the hash already exist before you do the test. That just leaves one line of addCookieToHash for later on which you can then inline too.

Comment 3

[Misak Khachatryan]

(In reply to comment #2)
> >-      if (/^https?:\/\/(?:[^@\/\s]+@)?([\w.-]+)/.test(aEntry.url) &&
> >-        !hosts[RegExp.$1] && _this._checkPrivacyLevel(_this._getURIFromString(aEntry.url).schemeIs("https"))) {
> >+      if (/^https?:\/\/(?:[^@\/\s]+@)?([\w.-]+)/.test(aEntry.url)) {
> >+        if (!hosts[RegExp.$1] && _this._checkPrivacyLevel(_this._getURIFromString(aEntry.url).schemeIs("https"))) {
> Not sure why this was changed from an && to a nested if?
> (If you still want to change it, I would also change the
> _this._getURIFromString(aEntry.url).schemeIs("https") to
> aEntry.url.substr(0, 5) == "https" to simplify the code)
> (or if you're really sneaky use extra capturing brackets
> to grab the https into RegExp.$1 and test that instead.)
> 

Here is the Simon's comment about it: https://bugzilla.mozilla.org/show_bug.cgi?id=423132#c1

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"))) {

Comment 4

[Misak Khachatryan]

Attachment: v2

Fixed all Your comments, except the if splitting, waiting Your decision about Simon's comment.

Comment 5

[Misak Khachatryan]

Well, I'm very sorry, but most of Your comments are already addressed in my ports of followup bugs. See  Bug 507587 and  Bug 507883. If You like how Your comments addressed there, I'll provide a consolidated patch. Sorry :(

Comment 6

[[e-mail]]

(In reply to comment #3)
Oh, I see what's going on now, this is the code:

if (some(thing)) {
  if (extra(thing))
    hosts[RegExp.$1] = true;
}
else if (other(thing)) {
  hosts[RegExp.$1] = true;
}

Comment 7

[[e-mail]]

(In reply to comment #5)
> Well, I'm very sorry, but most of Your comments are already addressed in my
> ports of followup bugs. See  Bug 507587 and  Bug 507883. If You like how Your
> comments addressed there, I'll provide a consolidated patch. Sorry :(
Well, bug 507883 has to be fixed one way or the other, but I prefer this bug's version of the fix for bug 507587, except for one issue which I'll comment on.

Comment 8

[[e-mail]]

Comment on attachment 427926 [details] [diff] [review]
v2

>+            // lazily build up a 3-dimensional hash, with
>+            // aHost, aPath, and aName as keys
Nit: they're called host, path and name now!

>+            if (!jscookies[cookie.host])
>+              jscookies[cookie.host] = {};
>+            if (!jscookies[cookie.host][cookie.path])
>+              jscookies[cookie.host][cookie.path] = {};
Somewhere along the line the test for
!jscookies[cookie.host][cookie.path][cookie.name]
got lost. It replaces the previous !isInHash test.

Comment 9

[[e-mail]]

Well actually there were two tests in the old code, but bug 507587's test was only an unwanted warning, while bug 507883's test was the unwanted one, but I can see how moving the code from addCookieToHash would have confused matters.

Comment 10

[Misak Khachatryan]

Attachment: v3

add missing check, other comments fixed.

Comment 11

[[e-mail]]

Comment on attachment 428043 [details] [diff] [review]
v3

>+    var cm = Components.classes["@mozilla.org/cookiemanager;1"].getService(Components.interfaces.nsICookieManager2);
I've just noticed that restoreCookies calls this cookieManager, and wraps the getService on to a second line, too. r+sr=me with this nit fixed.

Comment 12

[Misak Khachatryan]

Attachment: for checkin [Checkin: Comment 13]

for checkin, nits fixed, carrying forward r+ and sr+ from Neil.

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 428047 [details] [diff] [review]
for checkin
[Checkin: Comment 13]


http://hg.mozilla.org/comm-central/rev/5127e8c234b1