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
21 days ago
8 days ago

People

(Reporter: florian, Assigned: ttaubert)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

(Reporter)

Description

21 days ago
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

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

Comment 2

21 days 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

21 days 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

Comment 5

20 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ec044ae4bb52
Status: ASSIGNED → RESOLVED
Last Resolved: 20 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.3 - Apr 17
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [photon] → [photon-performance]
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.