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

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Session Restore
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Trunk
seamonkey2.1a1
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

9.56 KB, patch
Misak Khachatryan
: review+
Misak Khachatryan
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 425791 [details] [diff] [review]
patch with test
Attachment #425791 - Flags: superreview?(neil)
Attachment #425791 - Flags: review?(neil)

Updated

7 years ago
Attachment #425791 - Flags: superreview?(neil)
Attachment #425791 - Flags: superreview-
Attachment #425791 - Flags: review?(neil)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
(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"))) {
(Assignee)

Comment 4

7 years ago
Created attachment 427926 [details] [diff] [review]
v2

Fixed all Your comments, except the if splitting, waiting Your decision about Simon's comment.
Attachment #425791 - Attachment is obsolete: true
Attachment #427926 - Flags: superreview?(neil)
Attachment #427926 - Flags: review?(neil)
(Assignee)

Comment 5

7 years ago
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 :(
(Assignee)

Updated

7 years ago
Depends on: 507587, 507883

Comment 6

7 years ago
(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

7 years ago
(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

7 years ago
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

7 years ago
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.
(Assignee)

Comment 10

7 years ago
Created attachment 428043 [details] [diff] [review]
v3

add missing check, other comments fixed.
Attachment #427926 - Attachment is obsolete: true
Attachment #428043 - Flags: superreview?(neil)
Attachment #428043 - Flags: review?(neil)
Attachment #427926 - Flags: superreview?(neil)
Attachment #427926 - Flags: review?(neil)
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.
Attachment #428043 - Flags: superreview?(neil)
Attachment #428043 - Flags: superreview+
Attachment #428043 - Flags: review?(neil)
Attachment #428043 - Flags: review+
(Assignee)

Comment 12

7 years ago
Created attachment 428047 [details] [diff] [review]
for checkin
[Checkin: Comment 13]

for checkin, nits fixed, carrying forward r+ and sr+ from Neil.
Attachment #428043 - Attachment is obsolete: true
Attachment #428047 - Flags: superreview+
Attachment #428047 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Comment on attachment 428047 [details] [diff] [review]
for checkin
[Checkin: Comment 13]


http://hg.mozilla.org/comm-central/rev/5127e8c234b1
Attachment #428047 - Attachment description: for checkin → for checkin [Checkin: Comment 13]
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
You need to log in before you can comment on or make changes to this bug.