Closed Bug 1512990 (ipc-devirt) Opened Last year Closed 10 months ago

devirtualize Recv/Alloc/Dealloc protocol methods

Categories

(Core :: IPC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: froydnj, Assigned: Alex_Gaynor)

References

(Blocks 4 open bugs)

Details

Attachments

(5 files, 1 obsolete file)

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.).
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.
Depends on: 1513911
Priority: -- → P3
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.
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
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.
Depends on: 1515437
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.
Updated title, Send* methods aren't virtual as far as I can tell.
Summary: devirtualize Send/Recv protocol methods → devirtualize Recv/Alloc/Dealloc protocol methods
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.
(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.
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.
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.

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.

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

Depends on D16491

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?

Assignee: nobody → agaynor

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

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

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

Attachment #9036414 - Attachment is obsolete: true
Keywords: checkin-needed

Ryan tried to land this and got:

Failed to Land
On Tue, February 5, 2019, 5:29 PM GMT+2, by rvandermeulen@mozilla.com.
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', '')

Flags: needinfo?(agaynor)
Keywords: checkin-needed

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

Flags: needinfo?(agaynor) → needinfo?(apavel)

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

Flags: needinfo?(apavel)
Keywords: checkin-needed

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', '')

Flags: needinfo?(agaynor)
Keywords: checkin-needed

Ok, conflicts resolved!

Flags: needinfo?(agaynor)
Keywords: checkin-needed

Pushed by rvandermeulen@mozilla.com:
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

Keywords: checkin-needed
Alias: ipc-devirt
Blocks: 1526019
No longer depends on: 1513911
Blocks: 1529686
Depends on: 1537586
You need to log in before you can comment on or make changes to this bug.