1350640 - Eliminate the PContent::Msg_GetBlocklistState sync IPC

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P2)

Description

[(no longer active)]

The median time for this one is 15ms.

Comment 1

[Benjamin Smedberg]

I suspect this only applies to the plugin blocklist, since addon blocklisting would be handled entirely in Chrome. How often/what times does this message happen? Every time we attempt to instantiate a plugin instance?

Possible solutions:

A. calculate blocklist state for plugins in Chrome and send it over/store it with the plugin tags.
B. send over the entire plugin blocklist data to content and calculate it on demand there

Comment 2

[(no longer active)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> I suspect this only applies to the plugin blocklistsince addon
> blocklisting would be handled entirely in Chrome.

Yes I think so, this comes from nsPluginTag::GetBlocklistState() <http://searchfox.org/mozilla-central/rev/6e1c138a06a80f2bb3167d9dac050cc0fd4fb39e/dom/plugins/base/nsPluginTags.cpp#773>.

> How often/what times does
> this message happen? Every time we attempt to instantiate a plugin instance?

Based on some cursory look through searchfox:

 * When the page calls: navigator.plugins
 * From nsObjectLoadingContent::ShouldPlay() for click to play

But also from many other places, it's hard to follow all of the callers.

It's unfortunate that this cost is incurred even in the cases where the plugin loading cost isn't.

> Possible solutions:
> 
> A. calculate blocklist state for plugins in Chrome and send it over/store it
> with the plugin tags.
> B. send over the entire plugin blocklist data to content and calculate it on
> demand there

I don't know anything about this code so I don't have any opinions.  :-)

Comment 3

[:Felipe Gomes (needinfo for replies!)]

As I started to look into this code, I saw that each content process caches the result of the blocklist state already, so that this sync call only happens once per content-process (or again if there's a blocklist update notification).

The call here [1] returns successfully, so that the early return on the next line is skipped. Then this value is cached at line 795, and then subsequent calls immediately return the cached value on line 766.

Given this, is this bug still a p1? Or does its importance change?

It looks like doing the work in advance to send this down to the content process will only move the delay around, and possibly unnecessarily calculate the blocklist state in cases when it would not be needed (for CTP plugins for example). So the only thing that would be saved is the sync IPC overhead.

[1] http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/dom/plugins/base/nsPluginTags.cpp#773

Comment 4

[(no longer active)]

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> As I started to look into this code, I saw that each content process caches
> the result of the blocklist state already, so that this sync call only
> happens once per content-process (or again if there's a blocklist update
> notification).

Oh, I hadn't realized this is a 1 time cost (not sure how frequent blocklist updates are?)

> The call here [1] returns successfully, so that the early return on the next
> line is skipped. Then this value is cached at line 795, and then subsequent
> calls immediately return the cached value on line 766.
> 
> Given this, is this bug still a p1? Or does its importance change?

Did you check telemetry?

Here is the current data that I see:

Start	End	IPC_SYNC_MAIN_LATENCY_MS Count
0	32	1.28M (87.79%)
32	35	13.47k (0.92%)
35	38	10.37k (0.71%)
38	41	9.39k (0.64%)
41	45	10.58k (0.73%)
45	49	9.44k (0.65%)
49	53	8.08k (0.55%)
53	58	8.49k (0.58%)
58	63	7.52k (0.52%)
63	68	6.85k (0.47%)
68	74	6.92k (0.47%)
74	80	6.03k (0.41%)
80	87	6.18k (0.42%)
87	95	5.96k (0.41%)
95	103	5k (0.34%)
103	112	5.09k (0.35%)
112	122	4.78k (0.33%)
122	132	4.11k (0.28%)
132	143	4.03k (0.28%)
143	155	3.64k (0.25%)
155	168	3.39k (0.23%)
168	183	3.21k (0.22%)
183	199	2.98k (0.2%)
199	216	2.71k (0.19%)
216	235	2.6k (0.18%)
235	255	2.37k (0.16%)
255	277	2.27k (0.16%)
277	301	2.08k (0.14%)
301	327	1.8k (0.12%)
327	355	1.72k (0.12%)
355	386	1.58k (0.11%)
386	419	1.41k (0.1%)
419	455	1.29k (0.09%)
455	495	1.14k (0.08%)
495	538	1.1k (0.08%)
538	585	971 (0.07%)
585	636	885 (0.06%)
636	691	729 (0.05%)
691	750	653 (0.04%)
750	Infinity	7.05k (0.48%)

The pause times are fast in 87% of the cases but there is a *long* tail.  Pay attention to the last bucket.  :-(

I think the frequency of this changing opens it up to a retriage, for sure.  But just to clarify what we are talking about, we _are_ still talking about potential pauses for hundreds of milliseconds during a page load, just with a very low frequency.  But if most of our users for example use sessions with a short length of lifetime, then the frequency would be higher (since this will hit them "the first time" more times, etc.)

Given that I don't think there is a practical upper bound to how long sync IPCs like this can take, I'm personally going to argue for P1 again, I think.  :-)

> It looks like doing the work in advance to send this down to the content
> process will only move the delay around, and possibly unnecessarily
> calculate the blocklist state in cases when it would not be needed (for CTP
> plugins for example). So the only thing that would be saved is the sync IPC
> overhead.

First question, how many plugins are we talking about here?  Is it just Flash (aka, 1)?  Or am I being too naive?!  :-)

Also, how expensive is calculating the blocklist state?  And how large is the data?  I think we should try to move to a model where the parent tells each content process about the blocklist state at creation time and at update times and the content process never blocks on the parent for this information, unless if the above costs make that impractical.

Comment 5

[Naveed Ihsanullah [:naveed]]

Even though it is a one time cost it can be an unbounded wait so still P1.

Comment 6

[Andrew Overholt [:overholt]]

Felipe, are you able to get back to this given comment 5?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Yeah, I'll work on it in the next couple of days

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Kyle, since you've been working on bug 1337058, would you be interested in stealing this bug from me? Last I spoke with Benjamin, he suggested that I should be piggybacking on the FindPlugins message to pass this blocklist state info down to the content process.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I think we can do something similar to how the FindPlugins async solution works, i.e. send blocklist over on content process creation, then send again whenever the blocklist-updated notification is fired.

Comment 10

[Benjamin Smedberg]

Do we need to send the full blocklist data over, or just the blocklist state of individual plugin tags?

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

The GetBlocklistState IPC message, as it is right now, only sends over blocklists for a specific plugin tag, so we should just be able to bundle those into SetPluginList. I would think that any time the blocklist changed, we'd need to reload plugins anyways, so that message will always come with all the plugin information. Does this sound correct?

Comment 12

[Benjamin Smedberg]

I don't think we actually reload plugins, but I still think this plan would work fine.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1350640 - Send blocklist state on plugin list update;

Instead of synchronously checking the blocklist, package each plugin's
blocklist state with it when sending the information to the content
process. Whenever the blocklist is changed, just resend the whole
plugin list.

Review commit: https://reviewboard.mozilla.org/r/158024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158024/

Comment 14

[Benjamin Smedberg]

Comment on attachment 8887228 [details]
Bug 1350640 - Send blocklist state on plugin list update;

https://reviewboard.mozilla.org/r/158024/#review163560

\o/

Please make sure a followup is on file: I believe that we no longer need nsBlocklistServiceContent.js and should remove that code and perhaps assert that nobody ever tries to get the blocklist service in content.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bc1c884659c
Send blocklist state on plugin list update; r=bsmedberg

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1bc1c884659c