Closed
Bug 1353533
Opened 7 years ago
Closed 7 years ago
Creating empty maps while deleting non-existent cookies from SessionCookies.jsm's CookieStore causes jank near startup
Categories
(Firefox :: Session Restore, defect, P1)
Firefox
Session Restore
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: florian, Assigned: ttaubert)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [photon-performance])
Attachments
(1 file)
3.71 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Ahhhh, yeah. This is terrible. I'll take it.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8854794 -
Flags: review?(mdeboer)
Comment 3•7 years ago
|
||
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+
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
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec044ae4bb52
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon] → [photon-performance]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Comment 6•7 years ago
|
||
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•7 years 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.
Comment 8•7 years ago
|
||
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. :-)
Reporter | ||
Updated•7 years ago
|
No longer blocks: photon-performance-triage
Reporter | ||
Updated•7 years ago
|
Blocks: photon-performance-triage
You need to log in
before you can comment on or make changes to this bug.
Description
•