545455 - IPDL: Add hooks for when RPCChannel code is first pushed on and finally popped from the C++ stack

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Two uses so far

 (1) Enable/disable nested glib event loop detection, for better power efficiency
 (2) Defer Dealloc*() in IPDL until IPDL code is off the stack (mostly)

I really only care about hooking RPCChannel because that's all we've used so far, but a similar hook could be useful for SyncChannel.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 1: Track when RPCChannel code is first pushed on the C++ stack and last popped

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 2: Expose override-able hooks for top-level IPDL actors to be notified on stack entrance/exit

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: IPDL/C++ test

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 426608 [details] [diff] [review]
Part 1: Track when RPCChannel code is first pushed on the C++ stack and last popped

>+    void ExitedCxxStack();
>+    class NS_STACK_CLASS CxxStackFrame {

Nit: Newline between those

>+        CxxStackFrame(RPCChannel& that) : mThat(that) {

How about also AssertWorkerThread'ing here and in the dtor

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 426609 [details] [diff] [review]
Part 2: Expose override-able hooks for top-level IPDL actors to be notified on stack entrance/exit

> RPCChannel::EnteredCxxStack()
> RPCChannel::ExitedCxxStack()

Looks like these can be moved to the header (inlined)

>+        virtual void OnEnteredCxxStack() = 0;
>+        virtual void OnExitedCxxStack() = 0;

I think you should have a default impl that aborts here rather than make each non-toplevel actor implement their own.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ooh, ooh! Can we also make these RPCStack helpers record message name?! Maybe only in debug builds?!

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> (From update of attachment 426609 [details] [diff] [review])
> > RPCChannel::EnteredCxxStack()
> > RPCChannel::ExitedCxxStack()
> 
> Looks like these can be moved to the header (inlined)
> 
> >+        virtual void OnEnteredCxxStack() = 0;
> >+        virtual void OnExitedCxxStack() = 0;
> 
> I think you should have a default impl that aborts here rather than make each
> non-toplevel actor implement their own.

Did you mean to leave the review status as "?"?  Do you want to see the updated patch?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #6)
> Ooh, ooh! Can we also make these RPCStack helpers record message name?! Maybe
> only in debug builds?!

I have plans to add a Message::name() interface (in opt builds too).  Would that suit your needs?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 426609 [details] [diff] [review]
Part 2: Expose override-able hooks for top-level IPDL actors to be notified on stack entrance/exit

Oops

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #8)
> I have plans to add a Message::name() interface (in opt builds too).  Would
> that suit your needs?

I just want to be able to look at some variable in the debugger that has a stack of message names. I don't want to have to call any functions (can't do that in record+replay) to do it either. Just a simple const char* for each rpcframe would do it i think. And only in DEBUG builds.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OIC.  Like storing a std::stack<const char*> as a member of RPCChannel, and having CxxStackFrame push Message::name() onto that as part of the ctor?  That's certainly doable.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/projects/electrolysis/rev/5ca3dfc1266a
http://hg.mozilla.org/projects/electrolysis/rev/9800842b1f02
http://hg.mozilla.org/projects/electrolysis/rev/642cb539eb88

Comment 13

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/dbfb36b8b381
http://hg.mozilla.org/mozilla-central/rev/d0f27565024d
http://hg.mozilla.org/mozilla-central/rev/569dede83071