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)
Comment 1•7 years ago
|
||
![]() |
||
Updated•7 years ago
|
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 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•7 years ago
|
||
Assignee | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Comment 15•7 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•6 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•6 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•6 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•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 19•6 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•6 years ago
|
||
Can you say what revision of autoland that was trying to merge into?
Assignee | ||
Comment 21•6 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•6 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•6 years ago
|
Assignee | ||
Comment 23•6 years ago
|
||
Ok, conflicts resolved!
Comment 24•6 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•6 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•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Description
•