1553867 - Time spent in ClonePrincipalForPermission() during pageload

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

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()

Comment 1

[Andrea Marchesini [:baku]]

Attachment: Bug 1553867 - Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r=Ehsan

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 3

[Andrea Marchesini [:baku]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daf113171d63
Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r=Ehsan

Comment 5

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/daf113171d63

Comment 6

[Cosmin Sabou [:CosminS]]

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

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

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

Comment 9

[:gerard-majax]

(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.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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!!

Comment 12

[Andrea Marchesini [:baku]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cf2e5777de8
Reduce the number of ClonePrincipalForPermission() call in CookieSettings, r=Ehsan

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4cf2e5777de8

Comment 15

[Florin Strugariu [:Bebe]]

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