Last Comment Bug 524371 - Port Bug 423132 [speed up sessionstore cookie bits] to SeaMonkey
: Port Bug 423132 [speed up sessionstore cookie bits] to SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Misak Khachatryan
:
:
Mentors:
Depends on: 423132 507587 507883
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-25 11:50 PDT by Misak Khachatryan
Modified: 2010-02-21 16:10 PST (History)
0 users
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch with test (9.85 KB, patch)
2010-02-08 06:03 PST, Misak Khachatryan
neil: superreview-
Details | Diff | Splinter Review
v2 (9.85 KB, patch)
2010-02-20 02:12 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review
v3 (9.53 KB, patch)
2010-02-21 05:46 PST, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Splinter Review
for checkin [Checkin: Comment 13] (9.56 KB, patch)
2010-02-21 06:13 PST, Misak Khachatryan
misak.bugzilla: review+
misak.bugzilla: superreview+
Details | Diff | Splinter Review

Description Misak Khachatryan 2009-10-25 11:50:34 PDT
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 2010-02-08 06:03:38 PST
Created attachment 425791 [details] [diff] [review]
patch with test
Comment 2 neil@parkwaycc.co.uk 2010-02-10 08:41:11 PST
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 2010-02-10 23:39:11 PST
(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 2010-02-20 02:12:39 PST
Created attachment 427926 [details] [diff] [review]
v2

Fixed all Your comments, except the if splitting, waiting Your decision about Simon's comment.
Comment 5 Misak Khachatryan 2010-02-20 03:48:30 PST
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 neil@parkwaycc.co.uk 2010-02-20 07:49:26 PST
(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 neil@parkwaycc.co.uk 2010-02-20 07:53:05 PST
(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 neil@parkwaycc.co.uk 2010-02-20 07:58:01 PST
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 neil@parkwaycc.co.uk 2010-02-20 08:01:50 PST
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 2010-02-21 05:46:06 PST
Created attachment 428043 [details] [diff] [review]
v3

add missing check, other comments fixed.
Comment 11 neil@parkwaycc.co.uk 2010-02-21 05:57:29 PST
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 2010-02-21 06:13:29 PST
Created attachment 428047 [details] [diff] [review]
for checkin
[Checkin: Comment 13]

for checkin, nits fixed, carrying forward r+ and sr+ from Neil.
Comment 13 Serge Gautherie (:sgautherie) 2010-02-21 16:09:08 PST
Comment on attachment 428047 [details] [diff] [review]
for checkin
[Checkin: Comment 13]


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

Note You need to log in before you can comment on or make changes to this bug.