Sending permissions for service workers during content process initialization can be very slow

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mconley, Assigned: Nika)

Tracking

({perf, regression})

unspecified
mozilla56
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments, 1 obsolete attachment)

I just noticed some jank in my browser and captured this profile:

http://bit.ly/2swvv8V

We're spending about 300ms sending permissions for service workers down to the content process here.

We should probably do less of that.

I sent the list of service workers I have registered to mystor for reference.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1354700
(Assignee)

Updated

2 years ago
Assignee: nobody → michael
(Assignee)

Updated

2 years ago
Whiteboard: [qf]
(Assignee)

Comment 2

2 years ago
This should change the logic to completely avoid parsing any principals during the hot codepath.

MozReview-Commit-ID: 28BCIqA2Kf2
Attachment #8879732 - Flags: review?(ehsan)

Updated

2 years ago
Attachment #8879732 - Flags: review?(ehsan) → review+

Comment 3

2 years ago
This needs to be backported to 55, I think.  The profile in comment 0 is from 27 service workers.

Updated

2 years ago
Blocks: 1353179
status-firefox55: --- → affected
status-firefox56: --- → affected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Keywords: perf, regression
(Assignee)

Comment 4

2 years ago
Did some minor fixups due to try failures :)

MozReview-Commit-ID: 28BCIqA2Kf2
(Assignee)

Updated

2 years ago
Attachment #8879732 - Attachment is obsolete: true

Comment 5

2 years ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44532a19e524
Stop parsing principals during GetPermissionsForKey, r=ehsan
(Assignee)

Comment 6

2 years ago
The patch cherry-picked cleanly on beta, but I'm re-attaching it anyway :)

MozReview-Commit-ID: 28BCIqA2Kf2
I had to back this out because it or something else from the push caused bug 1375293, which was failing more frequently than we could handle.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cbeb4bc60daa798c60102102c15e51a5af94265a
Flags: needinfo?(michael)

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44532a19e524
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 9

2 years ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/62899ab59159
Backed out changeset 44532a19e524 for test_group_mouseevents.html failures a=backout
Status: RESOLVED → REOPENED
status-firefox56: fixed → ?
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---

Comment 10

2 years ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13422d457a38
Stop parsing principals during GetPermissionsForKey, r=ehsan

Updated

2 years ago
Whiteboard: [qf] → [qf:p1]

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/13422d457a38
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox56: ? → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 12

2 years ago
Comment on attachment 8880143 [details] [diff] [review]
BETA - Stop parsing principals during GetPermissionsForKey

Approval Request Comment
[Feature/Bug causing the regression]: bug 1353179
[User impact if declined]: Starting new content processes will be slower if you have a decent number of Service Workers.
[Is this code covered by automated tests?]: Not explicitly, but exercised heavily.
[Has the fix been verified in Nightly?]: Just landed
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Small risk comes from changing the way that we handle principals in the permission manager slightly. The permission manager is already quite conservative with its use of complex principals, so I don't think it is likely to have been super broken.
[String changes made/needed]: None
Flags: needinfo?(michael)
Attachment #8880143 - Flags: approval-mozilla-beta?
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected

Updated

2 years ago
Depends on: 1315092
Comment on attachment 8880143 [details] [diff] [review]
BETA - Stop parsing principals during GetPermissionsForKey

Should improve perf for service worker heavy pages - let's take this for beta 5.
Attachment #8880143 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/017218b8ecdc
status-firefox55: affected → fixed
tracking-firefox55: ? → +
tracking-firefox56: ? → +
Depends on: 1442784
No longer depends on: 1442784
You need to log in before you can comment on or make changes to this bug.