Closed Bug 1553867 Opened 7 months ago Closed 6 months ago

Time spent in ClonePrincipalForPermission() during pageload

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: jesup, Assigned: baku)

References

Details

(Whiteboard: [qf:p1:pageload])

Crash Data

Attachments

(1 file)

When loading cnn.com, I see ~1.5% of the Master Process time spent in nsPermission::Matches(), almost all in ClonePrincipalForPermission():
https://perfht.ml/2HReDBz
it's doing this as part of mozilla::AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor()

Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Whiteboard: [qf] → [qf:p1:pageload]

A very rough estimation from taking 1 profile with this patch is that the time in ClonePrincipal+MatchesPrincipal is perhaps 1/4 what it was. It still shows up (at ~0.5% and ~0.1% respectively). Note that the load time and number of samples are noticeably different from the profile earlier today.

https://perfht.ml/2VX7V77

1/4 is a good improvement, I hope. We can do something more in case we need to reduce more the impact of this method.

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daf113171d63
Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r=Ehsan
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1554527
Regressions: 1554567
Regressions: 1555166

Backed out as requested by Sylvestre for causing huge perf regressions. https://hg.mozilla.org/mozilla-central/rev/3a79d3be67486be1d30bda47988cf8c76ee3a3ee

Status: RESOLVED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---

Was the regression happening also with my patch for bug 1555166?

Flags: needinfo?(amarchesini) → needinfo?(sledru)

yeah, alexandre lissy was experiencing it with today's version too.
and other mentioned that the patch didn't fix it:
https://bugzilla.mozilla.org/show_bug.cgi?id=1554527#c10

Flags: needinfo?(sledru)

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)

yeah, alexandre lissy was experiencing it with today's version too.
and other mentioned that the patch didn't fix it:
https://bugzilla.mozilla.org/show_bug.cgi?id=1554527#c10

I've been running the respin with the backout for a few hours, received the update circa 12:00, it's now circa 22:00, and I'm confident the issue is not here anymore.

I've been running the respin with the backout for a few hours, received the update circa 12:00, it's now circa 22:00, and I'm confident the issue is not here anymore.

Thanks for the confirmation.

Baku, I'm very interested when we figure out why this patch had this surprising impact on perf. Did it cause extra IPC trips or validations? I want to understand in order to help provide advice on possible perf pitfalls one might not expect from a patch. Thanks!!

Flags: needinfo?(amarchesini)
Duplicate of this bug: 1555071
Crash Signature: [@ mozilla::net::CookieSettings::CookiePermission]
Flags: needinfo?(amarchesini)
Attachment #9067129 - Attachment description: Bug 1553867 - Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r?ehsan → Bug 1553867 - Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r=Ehsan

The reason behind this regression is because my patch clones the principal also when it was not needed. In particular, for permissions, we create a principals using just the origin of the URL. This is somehow slow.

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cf2e5777de8
Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r=Ehsan
Priority: -- → P2
Status: REOPENED → RESOLVED
Closed: 7 months ago6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

We got a small improvment on performance

== Change summary for alert #21264 (as of Mon, 03 Jun 2019 23:26:07 GMT) ==

Improvements:

4% raptor-tp6m-ebay-kleinanzeigen-search-geckoview-cold fcp android-hw-g5-7-0-arm7-api-16 opt 2,250.31 -> 2,167.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21264

You need to log in before you can comment on or make changes to this bug.