Creating empty maps while deleting non-existent cookies from SessionCookies.jsm's CookieStore causes jank near startup

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Session Restore
P1
normal
RESOLVED FIXED
2 months ago
15 days ago

People

(Reporter: florian, Assigned: ttaubert)

Tracking

(Blocks: 3 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

See this profile where 149ms of jank are spent deleting cookies from _updateCookie in SessionCookies.jsm: https://perf-html.io/public/ebdfb772af3c32bd7fb592ce8d0020a6b90de887/calltree/?jsOnly&range=3.6613_3.9458&thread=0

This is the code at http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/browser/components/sessionstore/SessionCookies.jsm#202 added in bug 1239671 for Firefox 45 and later.

This happens the first time the current session is saved (so a few seconds after the initial startup) while calling _ensureInitialized in SessionCookies.jsm.

The cookies being deleted here are the non-session cookies, which obviously aren't in the session cookies store, so for each of them _ensureMap uselessly creates empty map objects.

I think we should create a different code path for initializing SessionCookie.jsm, so that we can avoid calling _updateCookie here.

note: This jank can't be reproduced with a fresh profile, as its length depends on how many non-session cookies are stored in the profile.
(Assignee)

Comment 1

2 months ago
Ahhhh, yeah. This is terrible. I'll take it.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Blocks: 1330635
(Assignee)

Comment 2

2 months ago
Created attachment 8854794 [details] [diff] [review]
0001-Bug-1353533-Don-t-create-maps-for-non-session-cookie.patch
Attachment #8854794 - Flags: review?(mdeboer)
Comment on attachment 8854794 [details] [diff] [review]
0001-Bug-1353533-Don-t-create-maps-for-non-session-cookie.patch

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

LGTM, but you'll need to rebase your things in bug 912717 if you land this before that one. Up to you.
Attachment #8854794 - Flags: review?(mdeboer) → review+

Comment 4

2 months ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec044ae4bb52
Don't create maps for non-session cookies when reloading all cookies r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/ec044ae4bb52
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

a month ago
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon] → [photon-performance]

Updated

a month ago
Flags: qe-verify? → qe-verify-
We're trying to find the root cause of a 1.7ms in the 95th percentile input event responsiveness telemetry probe from the parent process in bug 1362094 and this patch is one of the three possible culprits that we currently suspect as the possible root causes.

Tim, do you think this could be the possible cause of bug 1362094?  Would you be OK with a temporary backout of this bug to see if the regression is reclaimed on telemetry?
Blocks: 1362094
(Assignee)

Comment 7

17 days ago
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #6)
> Tim, do you think this could be the possible cause of bug 1362094?  Would
> you be OK with a temporary backout of this bug to see if the regression is
> reclaimed on telemetry?

At first glance it seems that it shouldn't be able to cause a regression like this. I'd definitely be OK with a backout but unfortunately bug 912717 rewrote most of the file and touched that part of the code too. So we'd have to backout both.
If you think this isn't likely to have caused the regression and especially if it requires backing out bug 912717, let's sit on top of this one for now and examine the other two bugs as potential culprits...

Thanks for checking. :-)
No longer blocks: 1348289
Blocks: 1348289
You need to log in before you can comment on or make changes to this bug.