1366213 - Make the observer is called way less often in SessionCookies.jsm

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Will Wang [:WillWang]]

This is a follow-up bug for bug 1356605 comment 16/17:

"add a new observer notification in nsICookieService, called 'session-cookie-changed', that only fires when a session cookie was added/ removed/ updated. This'll make sure that the observer is called way less often in SessionCookies.jsm."

As bug 1356605 comment 3, this might ideally save 99% observer calling, and can also save all isSession() check in SessionCookies.jsm.

Comment 1

[Will Wang [:WillWang]]

== Existing design of session cookie ==

In the current implementation of nsICookieService[1], there are 5 types of notification for topic 'cookie-changed':
    added, changed, deleted, cleared, batch-deleted.

All of them are used as the bridge between session restore(SR) and necko(cookie service): keep the 'CookieStore'(of the SR) having the identical copy of session cookies in cookie service.

This design(keeping a copy) greatly improves the original inefficient way. (Ref bug 903398 for more info)

However, the downside of this sort of design is that we have to very carefully maintain the data consistency under every condition, otherwise it will breed bugs, which might be hard and cost time to be identified. One real example is bug 1221480 and bug 1239671.


== Goal of this bug ==

At first, this bug aim to reduce observer calling, the basic idea is in comment 1;
But after considering the issues of data consistency above and digging the interaction between necko and SR, I would tend to reduce observer calling cautiously here as a balance of performance and code complexity(/readability), to avoid introducing more strange bugs by someone in the future.

IMO, I'd like to optimize as follows:

In necko: add a new observer notification in nsICookieService, called 'session-cookie-changed', and for each type of notification of the topic:

1. added:
    make necko only fires notification for session cookies, as comment 1 said.

2. changed:
    This is the key part, which fires most notifications; and unfortunately, this is also the hard part to be reduced.
please refer to bug 1239671 comment 6: 
    "- when there is a session only cookie in the session store and a new value for the cookie is being set as persistent, the session stores cookie has to be deleted"
That is, *SR* need to examine *every* cookie whenever it's session cookie or not(persistent cookie)[2], since every persistent cookie might be the one we need to delete. So, basically, SR have to get every notification for each cookie.
One possible solution is, necko check whether every persistent cookie was already a session cookie saved in the hash table of the cookie service, then notify it; however, this will further complicate the logic of function 'nsCookieService::AddInternal' and make it sort of fragile.


3. deleted:
    make necko only fires notification for session cookies, as comment 1 said.

4. cleared:
    necko don't need to filter session cookies in this case.

5. batch-deleted:
    necko don't need to filter session cookies in this case.    


== Further design ==

With my limited experience, we can do some optimization here, but not too much; and there might be other better way to improve the performance further if needed, like create another hash table for session cookies(which should be pretty small) in the cookie service, though many other aspects should be considered as well.




[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieService
[2] http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/browser/components/sessionstore/SessionCookies.jsm#139-140
[3] http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/netwerk/cookie/nsCookieService.cpp#3586-3797

Comment 2

[Will Wang [:WillWang]]

(In reply to Will Wang [:WillWang] from comment #1)
> 2. changed:
>     This is the key part, which fires most notifications; and unfortunately,
> this is also the hard part to be reduced.
> please refer to bug 1239671 comment 6: 
>     "- when there is a session only cookie in the session store and a new
> value for the cookie is being set as persistent, the session stores cookie
> has to be deleted"
> That is, *SR* need to examine *every* cookie whenever it's session cookie or
> not(persistent cookie)[2], since every persistent cookie might be the one we
> need to delete. So, basically, SR have to get every notification for each
> cookie.
> One possible solution is, necko check whether every persistent cookie was
> already a session cookie saved in the hash table of the cookie service, then
> notify it; however, this will further complicate the logic of function
> 'nsCookieService::AddInternal' and make it sort of fragile.

Hi valentin :)

needinfo you since you are an experienced developer in necko, but at this moment I'm not sure you are the right person to ask, please feel free to redirect ni if needed :)

For this quoted part('changed' notification), could you imagine a more elegant way to filter out cookies?
I tend to not mess up the nsCookieService::AddInternal, however I might be wrong.

Thanks!

Comment 3

[Will Wang [:WillWang]]

Attachment: bug-1366213-WIP.patch

For reference:

Based on comment 1, here is the latest patch with dev logs which can be utilized by others if needed.

This patch also updates and pass the test in both necko and session restore.

Comment 4

[Will Wang [:WillWang]]

(In reply to Will Wang [:WillWang] from comment #1)
> == Goal of this bug ==
> 
> At first, this bug aim to reduce observer calling, the basic idea is in
> comment 1;
> But after considering the issues of data consistency above and digging the
> interaction between necko and SR, I would tend to reduce observer calling
> cautiously here as a balance of performance and code
> complexity(/readability), to avoid introducing more strange bugs by someone
> in the future.

Hi Mike,

I'm not sure whether my understanding is appropriate or not.
Before I attach the patch for review, could you help to share some thoughts if possible?
Please correct me if any, thanks a lot! :)

Comment 5

[Will Wang [:WillWang]]

Errata:

s/comment 1/comment 0/  for all the content of comment 1.

Sorry for confusing.

Comment 6

[Will Wang [:WillWang]]

(In reply to Will Wang [:WillWang] from comment #1)
> One possible solution is, necko check whether every persistent cookie was
> already a session cookie saved in the hash table of the cookie service, then
> notify it; however, this will further complicate the logic of function
> 'nsCookieService::AddInternal' and make it sort of fragile.

Thanks, Amy!

Per offline discussion, Amy has suggested a minimal modification:
add IsSession() check in function 'nsCookieService:: AddInternal',
then pass the check result to 'nsCookieService::NotifyChanged' as a newly added parameter.

I'll update the patch for this(case 'changed').

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Will Wang [:WillWang] from comment #6)
> Per offline discussion, Amy has suggested a minimal modification:
> add IsSession() check in function 'nsCookieService:: AddInternal',
> then pass the check result to 'nsCookieService::NotifyChanged' as a newly
> added parameter.

This sounds good to me! I'm not sure that this will introduce more complexity in the sessionstore component - I would think it would stay similar or in fact reduce complexity.
It's hard to say without seeing the code.
It sounds to me that you're on the right track here and good idea to talk with Amy ;-)

Comment 8

[Will Wang [:WillWang]]

Attachment: bug-1366213-WIP-v2.patch

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> This sounds good to me! I'm not sure that this will introduce more
> complexity in the sessionstore component - I would think it would stay
> similar or in fact reduce complexity.
These days I'm dealing with several session cookie bugs or collect time bugs, such as bug 1356605, bug 1353586 and this bug; thus I was thinking over some fundamental things, which might substantially reduce more complexity in the sessionstore component.

In bug 1356605, we have dramatically reduced the time of getting all session cookies from Necko's cookie service, like:
- 397 ms -> 13 ms, in an extremely slow device (bug 1356605 comment 33)
- 37 ms -> 2 ms, in a 2014 MacBook (bug 1356605 comment 11)

So I was wondering: if getting all session cookies just need 2ms [1], 
what if we don't maintain a copy(cookie store) anymore and just use the new 'Services.cookies.sessionEnumerator' within SessionCookies.collect() ?

This can:
1. reduce code complexity(also avoid introducing more strange bugs in the future (ref comment 1), reduce the time of reading code, ...
2. avoid all observer calling in SessionCookies.jsm

With my limited experience, it's a design change we could consider about.

[1] For the 2ms:
Necko takes 2ms to filter out session cookies from *all* cookies; If needed, we could further reduce the time. 
One of the possible solutions I could imagine: In the cookie hash table of necko, make every session cookie within each list(per web domain) is stored at the very beginning of the list. Thus the iteration could be speeded up by just looking the first few cookies.

I suppose that if we don't reduce the 2ms, the current design(return the copy in session restore) is faster in collect(< 2ms).

I tried to analysis the telemetry data for the time of _reloadCookies(), but it's not helpful for this case:
1. FX_SESSION_RESTORE_COLLECT_COOKIES_MS was removed since we don't need it.(bug 1359429)
2. Even we have FX_SESSION_RESTORE_COLLECT_COOKIES_MS data, but basically, it's about the time of each collect, not the first collect, which calls _reloadCookies().



> It's hard to say without seeing the code.
> It sounds to me that you're on the right track here and good idea to talk
> with Amy ;-)

Based on comment 6, here is the latest patch with dev logs which can be utilized if needed.
This patch passes the test in both necko and session restore.

During adding our logic to cookie service, I found many pitfalls, which means it's sort of "Magic! don't touch" code...
and thanks to the tests(especially the browser_cookies.js), I am more aware of those pitfalls and refine the logic in the patch.


Mike, could you help to suggest the patch and the design change? Many thanks!!

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Will Wang [:WillWang] from comment #8)
> In bug 1356605, we have dramatically reduced the time of getting all session
> cookies from Necko's cookie service, like:
> - 397 ms -> 13 ms, in an extremely slow device (bug 1356605 comment 33)
> - 37 ms -> 2 ms, in a 2014 MacBook (bug 1356605 comment 11)
> 
> So I was wondering: if getting all session cookies just need 2ms [1], 
> what if we don't maintain a copy(cookie store) anymore and just use the new
> 'Services.cookies.sessionEnumerator' within SessionCookies.collect() ?

This change would have the following effect:
Each time `collect()` is called, we iterate and collect all the session cookies, check if they conform to the current PrivacyLevel and convert them to JS objects. This will take longer as the amount of tabs/ session cookies grows over time, resulting in O(n) time complexity, at best.
The reason why the FX_SESSION_RESTORE_COLLECT_COOKIES_MS telemetry probe was removed is because the complexity for a `collect()` call was reduced to O(1), rendering it wasteful information.
With your suggestion we'd have to put the probe back, because the duration of a `collect()` would fluctuate again.
Please let me know if my explanation above is lacking, I'd like us both to understand the implications of your proposal! :-)

> Based on comment 6, here is the latest patch with dev logs which can be
> utilized if needed.
> This patch passes the test in both necko and session restore.
> 
> During adding our logic to cookie service, I found many pitfalls, which
> means it's sort of "Magic! don't touch" code...
> and thanks to the tests(especially the browser_cookies.js), I am more aware
> of those pitfalls and refine the logic in the patch.

To me your patch looks great and ready for review - after a bit of cleanup. I think that this optimization will _really_ make an impact for people who experience Firefox getting slower after long hours/ days/ weeks of usage. So your work here is great and important!

Comment 10

[Will Wang [:WillWang]]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> This change would have the following effect:
> Each time `collect()` is called, we iterate and collect all the session
> cookies, check if they conform to the current PrivacyLevel and convert them
> to JS objects. This will take longer as the amount of tabs/ session cookies
> grows over time, resulting in O(n) time complexity, at best.

Mike, thank you!

In brief, at this moment I think I can file a follow-up bug for further discussion/implementation, and push current patch(with cleanup) into review first.

We can restart the discussion from that follow-up bug whenever we have to modify the logic and find it taking too much time or buggy.

I agree with you about the O(n) time complexity, so my point in comment 8 also is:
"I suppose that if we don't reduce the 2ms, the current design(return the copy in session restore) is faster in collect(< 2ms)."

I was thinking over the perf in both cases, which could be the same time complexity(If I understand correctly):

1. current design(establish/maintain a copy in session restore): 
(a) During the Firefox running, check every cookie whether it's session cookie(either in cookie service(C++, what this patch did) or in session restore(JS)) then notify/add each session cookie
(b) Once collect, fetch them from cookie store.
==> (a) is sort of O(n) which is amortized during the Firefox running, (b) is O(1).

2. new design(use the new 'Services.cookies.sessionEnumerator'): 
If we can "make every session cookie within each list(per web domain) is stored at the very beginning of the list." as comment 8 said(or another smarter way which Necko developer knows):
(a) During the Firefox running, check every cookie whether it's session cookie, move it to the beginning of the list ...... O(n)
(b) Once collect, fetch them from the beginning of each list.  ...... I suppose it's O(1)
Besides,
- Again, this avoids all observer calling in SessionCookies.jsm.
- This also eliminates the time in bug 1356605(copy session cookies), however, we still need to sort lists after cookie DB init.

What do you think? :D


> To me your patch looks great and ready for review - after a bit of cleanup.
> I think that this optimization will _really_ make an impact for people who
> experience Firefox getting slower after long hours/ days/ weeks of usage. So
> your work here is great and important!

OK, I am going to request review.
I am still not sure how much the improvement will be, the observer callings are scattered over the profile.
Could you share the rough evaluation in short?

Thanks!

Comment 11

[Will Wang [:WillWang]]

Attachment: Bug 1366213: Part 1 - Add a new observer notification 'session-cookie-changed'.

Hi Josh, Based on the comments above I add a new observer notification 'session-cookie-changed' which will only be fired once a cookie update is related to the synchronization of the copy in session restore component. 

If I understand correctly, since this modification is basically adding a new observer notification without touching other logic, the original functions in cookie service should stay unchanged. Tests in necko are all passed.

Amy Chung suggested me that you are a great reviewer in this area, so I was wondering that could you help to review this part of the patch?

Many thanks!

Comment 12

[Will Wang [:WillWang]]

Attachment: Bug 1366213: Part 2 - Make session restore component switch to observe new topic 'session-cookie-changed' instead of 'cookie-changed'.

Hi Mike, here is the patch as our discussion, could you help to review?
Many thanks!

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 8882648 [details] [diff] [review]
Bug 1366213: Part 1 - Add a new observer notification 'session-cookie-changed'.

Review of attachment 8882648 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/nsCookieService.cpp
@@ +2256,5 @@
>    }
> +  // Notify for topic "private-cookie-changed" or "cookie-changed"
> +  os->NotifyObservers(aSubject, topic, aData);
> +
> +  // Notify for topic "session-cookie-changed" to update the copy of session

Should we have separate private-session-cookie-changed and session-cookie-changed notifications? Alternatively, should we just ignore private session cookies since they will not be restored?

@@ +2258,5 @@
> +  os->NotifyObservers(aSubject, topic, aData);
> +
> +  // Notify for topic "session-cookie-changed" to update the copy of session
> +  // cookies in session restore component.
> +  // See Bug 1366213 for the following condition

// Filter out notifications for individual non-session cookies.

Comment 14

[Will Wang [:WillWang]]

(In reply to Josh Matthews [:jdm] from comment #13)
> Should we have separate private-session-cookie-changed and
> session-cookie-changed notifications? Alternatively, should we just ignore
> private session cookies since they will not be restored?
Thanks for your review, Josh!
It seems that I better also ignore private session cookies here, as the previous logic did(Session restore only observes 'cookie-changed').


> // Filter out notifications for individual non-session cookies.
Got it, will do.

Thanks!!

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8882650 [details] [diff] [review]
Bug 1366213: Part 2 - Make session restore component switch to observe new topic 'session-cookie-changed' instead of 'cookie-changed'.

Review of attachment 8882650 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a green try run ;-) Thanks for this, Will!

Comment 16

[Will Wang [:WillWang]]

Attachment: (v2) Bug 1366213: Part 1 - Add a new observer notification 'session-cookie-changed'.

Hi Josh,

Here is the revised patch, could you help to review again?
Thanks! :)

Comment 17

[Will Wang [:WillWang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97a8f30f26e2

Comment 18

[Will Wang [:WillWang]]

Attachment: bug-1366213-part1-v2-rebase.patch

To ensure the patches are up-to-date to the m-c, part 1 of patches will be slightly rebased -- this rebase can be automated by Hg, no need to manually modify.

So I append it here if needed when landing patches.

Comment 19

[Will Wang [:WillWang]]

Hi, please help to check-in, thanks for the help! :)

(The rebased patch for part 1 is attached to comment 18 if needed)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/317663d3edc6
Part 1 - Add a new observer notification 'session-cookie-changed'. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a83b03c3da
Part 2 - Make session restore component switch to observe new topic 'session-cookie-changed' instead of 'cookie-changed'. r=mikedeboer

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/317663d3edc6
https://hg.mozilla.org/mozilla-central/rev/f7a83b03c3da

Comment 22

[Will Wang [:WillWang]]

Mike already provided all info I need :)