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

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years 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)

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.
Ahhhh, yeah. This is terrible. I'll take it.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
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
https://hg.mozilla.org/mozilla-central/rev/ec044ae4bb52
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
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-
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
(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: photon-performance-triage
You need to log in before you can comment on or make changes to this bug.