1512990 - (ipc-devirt) devirtualize Recv/Alloc/Dealloc protocol methods

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

One thing that came up at the IPC meeting last week was that it's a bit silly to have our generated IPDL classes implement all the Send/Recv machinery (and other bits as well?) as virtual functions, given that there is (almost) always a single implementing class.  We'd like to fix this state of affairs by making the relevant methods non-virtual, which should make things slightly faster/smaller, as well as saving vtable space.

The plan is to do this in a roughly analogous fashion to how the DOM bindings implement things, where the "abstract" generated code implementation can include the concrete implementation header.  There may be some groundwork to be done first (ensuring each protocol is implemented by a single class, some renaming to make conventions for names line up, etc. etc.).

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The other piece of cleanup is that that some of the implementation classes are defined entirely in .cpp files (e.g., CycleCollectWithLogs{Parent,Child}), so they'd need to have the class declarations moved into headers.  (Or some of them might be replaceable with async+returns instead.)

Another advantage of devirtualization is that actor classes can inherit callback implementations.  This is definitely useful for sharing behavior that's common to all/most process types with PGPU, which is upside-down (see bug 1321492 comment #3); I don't know if there are other use cases like that.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Looks like there's actually quite a few cases of multiple subclasses of an IPDL protocol. Rough cut query for this with semmle: https://mozilla.demo.semmle.com/query/1000125/

The actual count there is inflated, roughly the square of the actual count since it's showing both (subclass1, subclass2) and (subclass2, subclass1).

A lot of these are cases similar to bug 1513911 where it'd be less hassle and code if we used async returns.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Whoops, Jed pointed out that the Semmle page isn't publicly viewable. The query I used there is:

import cpp

from Class c, Class subc1, Class subc2
where
	(c.getName().matches("P%Parent") or c.getName().matches("P%Child")) and
	(c.getABaseClass().getName() = "IProtocol" or c.getABaseClass().getName() = "IToplevelProtocol") and
	subc1.getABaseClass+() = c and subc2.getABaseClass+() = c and subc1 != subc2
select c, subc1, subc2


the list of protocol classes with multiple subclasses is:

- PTextureParent
- PFileSystemRequestChild
- PBackgroundSDBRequestParent
- PCompositorBridgeParent
- PBackgroundIDBDatabaseRequestParent
- PQuotaUsageRequestParent
- PBackgroundLSRequestParent
- PBackgroundLSSimpleRequestParent
- PBackgroundIDBRequestParent
- PBackgroundMutableFileParent
- PBackgroundIDBFactoryRequestParent
- PDocAccessibleChild
- PIndexedDBPermissionRequestChild
- PBackgroundParent
- PBackgroundFileRequestParent
- PQuotaRequestParent
- PJavaScriptChild
- PJavaScriptParent
- PPluginModuleParent
- PBackgroundChild

I haven't dug into most of these, I've just started reviewing PFileSystemRequestChild

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

On further review, I think a lot of these aren't actually a problem. They're mostly cases of a single class directly inheriting the protocol, and implementing all the Recv methods, and then a bunch of subclasses which fill in various virtual methods on that class. These cases are fine for the devirtualization we want to do. A list of things that I think are still challenges is:

- PCompositorBridgeParent
- PDocAccessibleChild
- PBackgroundParent
- PBackgroundChild
- PJavaScriptChild
- PJavaScriptParent
- PPluginModuleParent
- PBackgroundMutableFileParent

I'm not super confident that I assessed all the other ones correctly though, so I'm going to try to improve the Semmle query.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Attachment: d0551871-4544-4473-acc7-443032e503d2.png

I'm sorry this is included as an image, it was the easiest way to extract all the relevant information.

Assuming I've computer'd correctly, this is now the list of 38 (each one is listed twice) IPC methods (Recv*, Alloc*, Dealloc*) that are overriden by multiple different subclasses of a single protocol. That is, these are the blockers to devirtualizing these calls.

The overwhelming majority are on PCompositorBridgeParent.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Updated title, Send* methods aren't virtual as far as I can tell.

Comment 7

[Alex Gaynor [:Alex_Gaynor]]

After thinking a little bit harder (and having started on refactoring some of the named protocols, naturally) I realized it's fine to have overrides as long as they all come from a single direct-descendent of a PProtocol(Parent|Child). This is the case for all but one of these, so once bug 1513911 is landed we're open to making this change. Which will require a bunch of other changes, but not inheritance related ones.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> Assuming I've computer'd correctly, this is now the list of 38 (each one is
> listed twice) IPC methods (Recv*, Alloc*, Dealloc*) that are overriden by

For future reference, there's also Answer*, for `intr` methods: https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/ipc/ipdl/ipdl/lower.py#225

`intr` is used heavily by NPAPI code, lightly by GMP, and nowhere else (besides unit tests) that `rg` can find, so it's likely not a problem for this.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Great catch! I re-ran the query with Answer added, and the only instance that popped was AnswerNPN_SetValue_NPPVpluginRequiresAudioDeviceChanges, which is part of PPluginModuleParent, which I'd already examined for some of it's Recv methods, and it's ok.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

I also did a quick measurement of how much memory we could hope to win here; if I counted right there's 3475 (impl class, virtual method) tuples, which should mean 3475 vtable slots * 8 bytes each => ~28KB in vtables.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

I've got a WIP patch, which I'll now get cleaned up and posted. Doing a talos try run, here's the performance results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ce7e6e2031d81fd8c28c3410bec53ab4f75555a1&framework=1&showOnlyComparable=1&showOnlyConfident=1&selectedTimeRange=172800

Almost all positive, which makes sense, I can't imagine how this would degrade performance. Frankly some of the high confidence wins are much greater than I'd have anticipated.

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1512990 - Part 1 - refactor IPDL compiler to centralize all Recv/Alloc/Dealloc calls on this; r?nika

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1512990 - Part 2 - implement direct calls in the IPDL compiler; r?froydnj

When calling a Recv/Alloc/Dealloc method on a (for now) whitelisted type, cast
`this` to the derived class.

Depends on D16491

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1512990 - Part 3 - enable direct calls for a number of core IPC actors; r?froydnj

Depends on D16492

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

Ok, so working (WIP) patches up. Looking for feedback on the following questions:

a) Currently the whitelist is in a dict in lower.py, is there a better place?
b) A reasonable heuristic would probably be "/".join(namespaces) + protocol.name[1:] + "Parent/Child.h". Does that seem objectionable?
c) For classes that don't meet this naming scheme, where's the right long term place for overrides to live, or should all classes just be forced to comply?

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #15)

Ok, so working (WIP) patches up. Looking for feedback on the following questions:

a) Currently the whitelist is in a dict in lower.py, is there a better place?

I think lower.py is ok, but it's possible we might want to put it all in a separate file? That's just bikeshed paint, though.

Alternatively, is there a reasonable way to say "We've converted the core DOM ipc bits, everything else that's current doesn't the old-style virtual functions (some explicit list) and everything else must follow the new style"?

b) A reasonable heuristic would probably be "/".join(namespaces) + protocol.name[1:] + "Parent/Child.h". Does that seem objectionable?

That seems fine.

c) For classes that don't meet this naming scheme, where's the right long term place for overrides to live, or should all classes just be forced to comply?

I think the latter.

I've been thinking about the talos results, and we probably need to do a better test, but let's assume for the moment that some of the significant improvements are actual improvements. That seems to me like either a) the compiler was able to significantly improve codegen for some of these send/recv methods (possible? maybe copying somehow got folded away?); or b) we're getting a small win, but we're getting that win times calling some methods waaaaay more than we were expecting to. And maybe there's scope for improving how many messages we send in the first place?

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1512990 - Part 3 - remove declarations of Alloc/Dealloc methods from IPDL protocol base class; r?froydnj

For cases where the class has direct calls (that is, we cast this to the
subclass before making the call) no longer declare Alloc/Dealloc methods on the
base class at all. This should ensure that slots for them are not generated in
vtables, and also allow the derived class to choose the method signature (e.g.
whether it wants to take something by reference or by value).

Depends on D16492

Comment 18

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1512990 - Part 4 - remove declarations of Recv/Answer methods from IPDL protocol base class; r?froydnj

For cases where the class has direct calls (that is, we cast this to the
subclass before making the call) no longer declare Recv/Answer methods on the
base class at all. This should ensure that slots for them are not generated in
vtables, and also allow the derived class to choose the method signature (e.g.
whether it wants to take something by reference or by value).

Depends on D18131

Comment 19

[Andreea Pavel [:apavel]]

Ryan tried to land this and got:

Failed to Land
On Tue, February 5, 2019, 5:29 PM GMT+2, by [e-mail].
Revisions: D16491 diff 58360 ← D16492 diff 58361 ← D18131 diff 58363 ← D18132 diff 58364
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpT_uNRS\npatching file dom/ipc/TabParent.h\nHunk #1 FAILED at 135\n1 out of 6 hunks FAILED -- saving rejects to file dom/ipc/TabParent.h.rej\nabort: patch failed to apply', '')

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

Can you say what revision of autoland that was trying to merge into?

Comment 21

[Alex Gaynor [:Alex_Gaynor]]

Ok, pretty sure I got the conflict (not 100% confident that I rebased on the right autoland, but we'll find out!)

Comment 22

[Cosmin Sabou [:CosminS]]

Part 1 was succesfully landed here https://hg.mozilla.org/integration/autoland/rev/300334bd78b398bf98c3a08d27558168ab6b5626 and I backed it out here https://hg.mozilla.org/integration/autoland/rev/838983f6042d3b16b162c827be1816ca56b9d892 because of conflicts when trying to land Part 2,3,4:

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmpEfCmN7\npatching file dom/ipc/TabParent.h\nHunk #1 FAILED at 135\n1 out of 6 hunks FAILED -- saving rejects to file dom/ipc/TabParent.h.rej\nabort: patch failed to apply', '')

Comment 23

[Alex Gaynor [:Alex_Gaynor]]

Ok, conflicts resolved!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d623e8eff5e3
Part 1 - refactor IPDL compiler to centralize all Recv/Alloc/Dealloc calls on this; r=nika
https://hg.mozilla.org/integration/autoland/rev/cb8d1788b802
Part 2 - implement direct calls in the IPDL compiler; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/09c24f46810f
Part 3 - remove declarations of Alloc/Dealloc methods from IPDL protocol base class; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/92091d537161
Part 4 - remove declarations of Recv/Answer methods from IPDL protocol base class; r=froydnj

Comment 25

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d623e8eff5e3
https://hg.mozilla.org/mozilla-central/rev/cb8d1788b802
https://hg.mozilla.org/mozilla-central/rev/09c24f46810f
https://hg.mozilla.org/mozilla-central/rev/92091d537161