Closed Bug 1350640 Opened 6 years ago Closed 6 years ago
Eliminate the PContent::Msg
_Get Blocklist State sync IPC
59 bytes, text/x-review-board-request
The median time for this one is 15ms.
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
Priority: -- → P2
(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. :-)
Assignee: nobody → felipc
Status: NEW → ASSIGNED
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  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.  http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/dom/plugins/base/nsPluginTags.cpp#773
(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  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.
Whiteboard: [qf:p1] → [qf]
Even though it is a one time cost it can be an unbounded wait so still P1.
Whiteboard: [qf] → [qf:p1]
Felipe, are you able to get back to this given comment 5?
Yeah, I'll work on it in the next couple of days
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.
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.
Assignee: felipc → kyle
Do we need to send the full blocklist data over, or just the blocklist state of individual plugin tags?
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?
I don't think we actually reload plugins, but I still think this plan would work fine.
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.
Attachment #8887228 - Flags: review?(benjamin) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1bc1c884659c Send blocklist state on plugin list update; r=bsmedberg
11 months ago
Performance Impact: --- → P1
You need to log in before you can comment on or make changes to this bug.