1343755 - Label runnables in netwerk/sctp/datachannel/

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Kershaw Chang [:kershaw]]

It seems we have a lot of runnables need to be labeled in DataChannel.cpp.


netwerk/sctp/datachannel/DataChannel.cpp

NS_DispatchToMainThread(WrapRunnable(nsCOMPtr<nsIThread>(mInternalIOThread), // found in mozilla::DataChannelConnection::~DataChannelConnection

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::CompleteConnect

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::SendDeferredMessages

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleOpenRequestMessage

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleAssociationChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleAssociationChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleAssociationChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::HandleStreamChangeEvent

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::OpenFinish

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::OpenFinish

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannelConnection::OpenFinish

NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL); // found in mozilla::DataChannelConnection::ReadBlob

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannel::StreamClosedLocked

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable( // found in mozilla::DataChannel::AppReady

NS_DispatchToMainThread(runnable); // found in mozilla::DataChannel::AppReady

NS_DispatchToMainThread(aMessage); // found in mozilla::DataChannel::SendOrQueue

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in DataChannelConnection

Summary:
 - Make DataChannelConnection inherit from NeckoTargetHolder
 - Initialize main thread event in DataChannelConnection's constructor
 - Get an event target by calling GetNeckoTarget and use it to dispatch runnables in DataChannelConnection.cpp.

I am not quite sure who should I ask to review.
:jesup, could you perhaps take a look? Thanks.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8877463 [details] [diff] [review]
Label runnables in DataChannelConnection

Review of attachment 8877463 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits... NeckoTargetHolder does end up doing an extra refcount cycle for each use (and adds a fair bit of duplicate code), compared to if we just stored an nsCOMPtr<> directly and used it with a "DispatchToTarget(mVirtualMainThread, message)" method that does the "if null, dispatch to mainthread" bit.   But it will work.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +1485,5 @@
>      } else {
>        LOG(("Unexpected state: %d", mState));
>      }
>      break;
> +  case SCTP_COMM_LOST: {

brace on it's own line (this isn't an if; grey area in the style guidelines).  Better yet, don't brace; comptr will die at end of switch (but perhaps conflicts with later local target, if so can keep the braces)

@@ +1492,5 @@
> +    nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();
> +    target->Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
> +                       DataChannelOnMessageAvailable::ON_DISCONNECTED,
> +                       this)));
> +    }

Indent braced code

@@ +1503,5 @@
> +    nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();
> +    target->Dispatch(do_AddRef(new DataChannelOnMessageAvailable(
> +                       DataChannelOnMessageAvailable::ON_DISCONNECTED,
> +                       this)));
> +    }

ditto on braces

@@ +2142,5 @@
>        }
>        if (channel->mFlags & DATA_CHANNEL_FLAGS_FINISH_OPEN) {
>          // We already returned the channel to the app.
>          NS_ERROR("Failed to send open request");
> +        nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();

don't we already have target as a local here?

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to Randell Jesup [:jesup] from comment #2)
> Comment on attachment 8877463 [details] [diff] [review]
> Label runnables in DataChannelConnection
> 
> Review of attachment 8877463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits... NeckoTargetHolder does end up doing an extra refcount cycle
> for each use (and adds a fair bit of duplicate code), compared to if we just
> stored an nsCOMPtr<> directly and used it with a
> "DispatchToTarget(mVirtualMainThread, message)" method that does the "if
> null, dispatch to mainthread" bit.   But it will work.
> 

Thanks for the review.

To save an extra refcount cycle, I think I can add a function "DispatchToNeckoTargetOrMainThread" in NeckoTargetHolder.
By doing this, we can get rid off some unnecessary local variables and annoying braces.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Add a helper function in NeckoTargetHolder for dispatching runnables directly

Please take a look at comment#2.

The way of using mNeckoTarget to dispatch is kind of annoying. We have to get a target by calling GetNeckoTarget() first, and then use it to dispatch.

I think we can add a helper function to dispatch runnables directly.

What do you think, Honza?

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8877959 [details] [diff] [review]
Add a helper function in NeckoTargetHolder for dispatching runnables directly

Review of attachment 8877959 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed

::: netwerk/ipc/NeckoTargetHolder.cpp
@@ +32,5 @@
> +  if (mNeckoTarget) {
> +    return mNeckoTarget->Dispatch(Move(aRunnable), aDispatchFlags);
> +  }
> +
> +  return  NS_DispatchToMainThread(Move(aRunnable), aDispatchFlags);

Before landing please consult bug 1361164 and bug 1365096 (:billm), we are replacing NS_GetMainThread with something smarter, not sure if NS_DispatchToMainThread is being replaced too or not.

nit: two spaces after return.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8877959 [details] [diff] [review]
> Add a helper function in NeckoTargetHolder for dispatching runnables directly

also, where is this used?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8877959 [details] [diff] [review]
Add a helper function in NeckoTargetHolder for dispatching runnables directly

Review of attachment 8877959 [details] [diff] [review]:
-----------------------------------------------------------------

I wasn't asked, but r+.

Honza: This would replace (lots of) instances of :
{
 nsCOMPtr<nsIEventTarget> target = GetNeckoTarget();
 target->Dispatch(some form of runnable);
}

with:

Dispatch(runnable);

If Dispatch() might create conflicts (I don't think it will), you could use DispatchToTarget() or DispatchToNeckoTarget()

::: netwerk/ipc/NeckoTargetHolder.h
@@ +28,5 @@
>    // Get event target for processing network events.
>    virtual already_AddRefed<nsIEventTarget> GetNeckoTarget();
> +  // When |mNeckoTarget| is not null, use it to dispatch the runnable.
> +  // Otherwise, dispatch the runnable to the main thread.
> +  nsresult DispatchToNeckoTargetOrMainThread(

I suggest Dispatch() -- what it dispatches to is up to the NeckoTargetHolder to figure out; the algorithm doesn't need to be in the method name. :-)

Comment 8

[Kershaw Chang [:kershaw]]

> ::: netwerk/ipc/NeckoTargetHolder.h
> @@ +28,5 @@
> >    // Get event target for processing network events.
> >    virtual already_AddRefed<nsIEventTarget> GetNeckoTarget();
> > +  // When |mNeckoTarget| is not null, use it to dispatch the runnable.
> > +  // Otherwise, dispatch the runnable to the main thread.
> > +  nsresult DispatchToNeckoTargetOrMainThread(
> 
> I suggest Dispatch() -- what it dispatches to is up to the NeckoTargetHolder
> to figure out; the algorithm doesn't need to be in the method name. :-)

Thanks for this suggestion. I should've also asked your opinion :)

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8877959 [details] [diff] [review]
> Add a helper function in NeckoTargetHolder for dispatching runnables directly
> 
> Review of attachment 8877959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed
> 
> ::: netwerk/ipc/NeckoTargetHolder.cpp
> @@ +32,5 @@
> > +  if (mNeckoTarget) {
> > +    return mNeckoTarget->Dispatch(Move(aRunnable), aDispatchFlags);
> > +  }
> > +
> > +  return  NS_DispatchToMainThread(Move(aRunnable), aDispatchFlags);
> 
> Before landing please consult bug 1361164 and bug 1365096 (:billm), we are
> replacing NS_GetMainThread with something smarter, not sure if
> NS_DispatchToMainThread is being replaced too or not.
> 

Thanks for pointing this out.
This should be changed to something like:

 nsCOMPtr<nsIEventTarget> mainTarget = GetMainThreadEventTarget();
 return mainTarget->Dispatch(Move(aRunnable), nsIEventTarget::DISPATCH_NORMAL);

I'll update the patch.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Add a helper function in NeckoTargetHolder for dispatching runnables directly, r=mayhemer, jesup

Carry r+ from reviewers.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in DataChannelConnection, r=jesup

As said in comment#7, this patch uses NeckoTargetHolder::Dispatch to replace NS_DispatchToMainThread() in DataChannel.cpp.

I think we don't need another review round since changes in this patch are quite straightforward.

Comment 12

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ad90918ac00811cd70733b11c0e2325c3c0a55

Comment 13

[Carsten Book [:Tomcat]]

seems there are conflicts with the 2nd patch like 

patching file netwerk/sctp/datachannel/DataChannel.cpp
Hunk #2 FAILED at 237
Hunk #12 FAILED at 2410
2 out of 15 hunks FAILED -- saving rejects to file netwerk/sctp/datachannel/DataChannel.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0002-Bug-1343755-Label-runnables-in-DataChannelConnection.patch

could you take a look, thanks!

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to Carsten Book [:Tomcat] from comment #13)
> seems there are conflicts with the 2nd patch like 
> 
> patching file netwerk/sctp/datachannel/DataChannel.cpp
> Hunk #2 FAILED at 237
> Hunk #12 FAILED at 2410
> 2 out of 15 hunks FAILED -- saving rejects to file
> netwerk/sctp/datachannel/DataChannel.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> 0002-Bug-1343755-Label-runnables-in-DataChannelConnection.patch
> 
> could you take a look, thanks!

Thanks.

I'll rebase this until bug 1372405 is landed.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: Label runnables in DataChannelConnection, r=jesup

Rebased.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77e551ec0d5f
Part 1: Add a helper function in NeckoTargetHolder for dispatching runnable. r=mayhemer, r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b23b5dce54f
Label runnables in DataChannelConnection. r=jesup

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77e551ec0d5f
https://hg.mozilla.org/mozilla-central/rev/9b23b5dce54f