devirtualize Recv/Alloc/Dealloc protocol methods
Categories
(Core :: IPC, enhancement, P3)
Tracking
()
People
(Reporter: froydnj, Assigned: Alex_Gaynor)
References
(Blocks 5 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.).
Comment 1•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
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.
Assignee | ||
Comment 3•5 years ago
|
||
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
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
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.
Assignee | ||
Comment 6•5 years ago
|
||
Updated title, Send* methods aren't virtual as far as I can tell.
Assignee | ||
Comment 7•5 years ago
|
||
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•5 years ago
|
||
(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.
Assignee | ||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
When calling a Recv/Alloc/Dealloc method on a (for now) whitelisted type, cast `this` to the derived class. Depends on D16491
Assignee | ||
Comment 14•5 years ago
|
||
Depends on D16492
Assignee | ||
Comment 15•5 years ago
|
||
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?
Reporter | ||
Comment 16•5 years ago
|
||
(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?
Assignee | ||
Comment 17•5 years ago
|
||
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
Assignee | ||
Comment 18•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 19•5 years ago
|
||
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', '')
Assignee | ||
Comment 20•5 years ago
|
||
Can you say what revision of autoland that was trying to merge into?
Assignee | ||
Comment 21•5 years ago
|
||
Ok, pretty sure I got the conflict (not 100% confident that I rebased on the right autoland, but we'll find out!)
Comment 22•5 years ago
|
||
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', '')
Updated•5 years ago
|
Assignee | ||
Comment 23•5 years ago
|
||
Ok, conflicts resolved!
Comment 24•5 years ago
|
||
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
Comment 25•5 years ago
|
||
bugherder |
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
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Description
•