1229589 - Make TCPSocket more efficient about removing stale input streams

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josh Matthews [:jdm]]

TCPSocket::NotifyCopyComplete only removes the first stream from the multiplex stream, even though we could have copied more than one. If that's the case, it notices that the queue is non-empty and attempts to copy them again. This is wasteful, and makes it harder to reason about the correctness of this code.

I propose we add an `nsTArray<nsCOMPtr<nsIInputStream>>` in `TCPSocket` for storing any data passed to Send while mAsyncCopierActive is true. In NotifyCopyComplete, we can remove all the streams from the multiplex stream, then append the items from the new array member, and everything should continue to Just Work.

We can ensure that it's working by running the tests:
./mach mochitest dom/network/tests/test_client_and_server_basics.html
./mach mochitest-e10s dom/network/tests/test_client_and_server_basics.html

Code: dom/network/TCPSocket.cpp

Comment 1

[Salah]

Hi, I would like to work on this bug. Can you assign it to me please.

Comment 2

[Josh Matthews [:jdm]]

Hi Salah! We tend to assign bugs to new contributors after the first patch is submitted. Here are the steps I expect for solving this bug:

* in dom/network/TCPSocket.h, we add a new member as described in the original comment.
* in dom/network/TCPSocket.cpp in TCPSocket::Send(nsIInputStream* aStream, uint32_t aByteLength), before appending to the multiplex stream we first check if mAsyncCopierActive is true. If so, we append to the new member instead.
* in TCPSocket::NotifyCopyComplete, instead of only removing the stream in position 0, we will want to continuously remove the first stream while the multiplex stream is not empty. After that, we should append all of the streams in the new member added in the first step, and clear out that new member (see the nsTArray API at https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?from=nstarray#2084 )

Does that make sense?

Comment 3

[Salah]

Okay here is what I have done. 


* in dom/network/TCPSocket.h, we add a new member as described in the original comment.


// Input stream machinery
  nsCOMPtr<nsIInputStreamPump> mInputStreamPump;
  nsCOMPtr<nsIScriptableInputStream> mInputStreamScriptable;
  nsCOMPtr<nsIBinaryInputStream> mInputStreamBinary;
++ nsTArray<nsCOMPtr<nsIInputStream>>


I have added the new member in the input stream as it is storing data passed to it. Im not sure if this is the correct place or if it needs to be defined as public first at the top.


in dom/network/TCPSocket.cpp in TCPSocket::Send(nsIInputStream* aStream, uint32_t aByteLength), before appending to the multiplex stream we first check if mAsyncCopierActive is true. If so, we append to the new member instead.


bool
TCPSocket::Send(nsIInputStream* aStream, uint32_t aByteLength)
{
  uint64_t newBufferedAmount = BufferedAmount() + aByteLength;
  bool bufferFull = newBufferedAmount > BUFFER_SIZE;
  if (bufferFull) {
    // If we buffered more than some arbitrary amount of data,
    // (65535 right now) we should tell the caller so they can
    // wait until ondrain is called if they so desire. Once all the
    // buffered data has been written to the socket, ondrain is
    // called.
    mWaitingForDrain = true;
  }

  if (mSocketBridgeChild) {
    // In the child, we just add the buffer length to our bufferedAmount and let
    // the parent update our bufferedAmount when the data have been sent.
    mBufferedAmount = newBufferedAmount;
    return !bufferFull;
  }

++if (mAsyncCopierActive == true) {
++    ???
++}



  if (mWaitingForStartTLS) {
    // When we are waiting for starttls, newStream is added to pendingData
    // and will be appended to multiplexStream after tls had been set up.
    mPendingDataAfterStartTLS.AppendElement(aStream);
  } else {
    mMultiplexStream->AppendStream(aStream);
  }

  EnsureCopying();



I have added the If statement to check if it is true. However I’m not sure how exactly I should append to the new member.

Appreciate any feedback.

Comment 4

[Seema Shad]

Attachment: bug1229589.patch

Hi Josh,

I have attached a patch for this bug, could you please provide some feedback.

Kind regards,
Seema Shad

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 8697028 [details] [diff] [review]
bug1229589.patch

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

Hi Seema! I apologize for being silent for so long - your messages came during a week-long conference, then there were the holidays, etc. This is a good start, and I've included some comments that should hopefully clarify the missing pieces. Let me know if anything else is unclear!

::: dom/network/TCPSocket.cpp
@@ +398,5 @@
>  TCPSocket::NotifyCopyComplete(nsresult aStatus)
>  {
>    mAsyncCopierActive = false;
> +
> +  while (mMultiplexStream->NotEmpty()) {

You'll want to use the methods from https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/nsIMultiplexInputStream.h?from=nsimultiplexinputstream#25 . There should be no need to include a counter here, though; we want to remove the 0th element until no elements remain.

@@ +404,5 @@
> +    mMultiplexStream->NotEmpty(i);
> +    i++;
> +  }
> +
> +  mPassedDataToSend->AppendElements;

This will need to be a loop that removes the 0th element from mPassedDataToSend and appends it to mMultiplexStream.

@@ +405,5 @@
> +    i++;
> +  }
> +
> +  mPassedDataToSend->AppendElements;
> +       

nit: trailing whitespace

@@ +891,5 @@
>      // and will be appended to multiplexStream after tls had been set up.
>      mPendingDataAfterStartTLS.AppendElement(aStream);
>    } else {
> +    if {mAsynCopierActive == true) {
> +      mPassedDataToSend->AppendStream(aStream);

nsTArray doesn't have an AppendStream; you'll want AppendElement instead (https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1593)

::: dom/network/TCPSocket.h
@@ +234,5 @@
>    bool mWaitingForStartTLS;
>    // The buffered data awaiting the TLS upgrade to finish.
>    nsTArray<nsCOMPtr<nsIInputStream>> mPendingDataAfterStartTLS;
> +  //Stores passed data to send while mAsyncCopierActive is true
> +  nsTArray<nsCOMPTr<nsIInputStream>>mPassedDataToSend;

nit: add a space after the >> here. A clearer name could be `mPendingDataWhileCopierActive`.

@@ +235,5 @@
>    // The buffered data awaiting the TLS upgrade to finish.
>    nsTArray<nsCOMPtr<nsIInputStream>> mPendingDataAfterStartTLS;
> +  //Stores passed data to send while mAsyncCopierActive is true
> +  nsTArray<nsCOMPTr<nsIInputStream>>mPassedDataToSend;
> +  

nit: trailing whitespace

Comment 6

[Amit Kr. Chandra [:wizard_A]]

Hi Josh,

I have started working on this...

Thanks,
Amit.

Comment 7

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

Hi Josh,

Here is what i have implemented so far.. plz tell me if i am headed the right direction....

Thanks,
Amit.

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8740618 [details] [diff] [review]
Bug-1229589_Implement_efficient_TCPSocket.patch

That's the right idea, but as you noticed we should be appending to the new array instead of mMultiplexStream in the new `else if` case that's been added.

Comment 9

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

Hi Josh,

I have made the changes and the tests mentioned are passing. Requesting you to kindly review the changes made.

Thanking You,
Amit.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8741135 [details] [diff] [review]
Bug-1229589_Implement_efficient_TCPSocket.patch

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

Good start! Sorry for the delay in reviewing the changes.

::: dom/network/TCPSocket.cpp
@@ +411,5 @@
> +  uint32_t countRemaining;
> +  nsresult rvRemaining = mMultiplexStream->GetCount(&countRemaining);
> +  NS_ENSURE_SUCCESS_VOID(rvRemaining);
> +
> +  do {

Let's just do a `while (countRemaining--) {` loop instead of calling GetCount each time.

@@ +915,5 @@
>      // and will be appended to multiplexStream after tls had been set up.
>      mPendingDataAfterStartTLS.AppendElement(aStream);
> +  } else if (mAsyncCopierActive) {
> +    // While the AsyncCopier is still active..
> +    mPendingDataWhileCopierActive.AppendElement(aStream);

We need to actually transfer the streams from mPendingDataWhileCopierActive to mMultiplexStream when the copier is no longer active in NotifyCopyComplete.

Comment 11

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

Hi Josh,

Thank you for the review comments. I have worked on them and the patch is attached.
Please check if the changes made would resolve the bug.

Thanks,
Amit.

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 8743483 [details] [diff] [review]
Bug-1229589_Implement_efficient_TCPSocket.patch

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

Looks good, thank you! You should apply for access to our tryserver so we can go through the process of getting this merged: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

::: dom/network/TCPSocket.cpp
@@ +415,5 @@
> +  while (countRemaining--) {
> +    mMultiplexStream->RemoveStream(0);
> +  }
> +
> +  if (!mAsyncCopierActive) {

No need for this check; we set it to false earlier.

Comment 13

[Amit Kr. Chandra [:wizard_A]]

Hi Josh,

Thank you very much for your comments. I did that change there since I was not very sure of what you meant in your last line in comment in comment 10. Please guide me as to whether i need to change the above code or let it be...

Moreover, I would require someone to vouch for me to get an access to the try-server. Since, this is my third bug, i was not very sure as to my eligibility for the same. Will you vouch for me, only if you feel appropriate??

Thanks a lot,
Amit.

Comment 14

[Josh Matthews [:jdm]]

The first line of NotifyCopyComplete is `mAsyncCopierActive = false;`, so the if check is not useful :) Please go ahead and remove. Also, I will be happy to vouch for you.

Comment 15

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

Thanks a lot Josh for vouching for me.. I'll file the bug after this bug is resolved. Thanks.. :)

Comment 16

[Josh Matthews [:jdm]]

Comment on attachment 8746531 [details] [diff] [review]
Bug-1229589_Implement_efficient_TCPSocket.patch

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

::: dom/network/TCPSocket.cpp
@@ +914,5 @@
>      // and will be appended to multiplexStream after tls had been set up.
>      mPendingDataAfterStartTLS.AppendElement(aStream);
> +  } else if (mAsyncCopierActive) {
> +    // While the AsyncCopier is still active..
> +    mPendingDataWhileCopierActive.AppendElement(aStream);

This patch is missing the code that transfer the elements of mPendingDataWhileCopierActive in NotifyCopyComplete...

Comment 17

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

I hope i did not mess up this time... Thanks... :)

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 8749228 [details] [diff] [review]
Bug-1229589_Implement_efficient_TCPSocket.patch

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

::: dom/network/TCPSocket.cpp
@@ +415,5 @@
> +  while (countRemaining--) {
> +    mMultiplexStream->RemoveStream(0);
> +  }
> +
> +  if (!mAsyncCopierActive) {

This is what I commented about last time: this check for !mAsyncCopierActive is redundant - it is always true because we set mAsyncCopierActive to false earlier in this method.

Comment 19

[Amit Kr. Chandra [:wizard_A]]

Appologies... My bad... So i will go ahead and remove the if condition. And, the rest of the code (while loop) stays the same or is there any other implementation of copying mPendingDataWhileCopierActive to mMultiplexStream??

Comment 20

[Josh Matthews [:jdm]]

Yes, the rest of the code looks fine to me. Apologies for the delay; I was at a conference last week, and then it was a holiday weekend.

Comment 21

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

Thanks Josh. Requesting your review...

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8755887 [details] [diff] [review]
Bug-1229589_Implement_efficient_TCPSocket.patch

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

::: dom/network/TCPSocket.cpp
@@ +415,5 @@
> +  while (countRemaining--) {
> +    mMultiplexStream->RemoveStream(0);
> +  }
> +
> +  if (!mAsyncCopierActive) {

This if check is still here. Was this a mistake, or were the previous comments unclear?

Comment 23

[Amit Kr. Chandra [:wizard_A]]

Attachment: Bug-1229589_Implement_efficient_TCPSocket.patch

HI Josh... Really very sorry for this.. probably some mistake somewhere.. Hence, this time i am attaching the text of the patch.. Sorry for the mess..

Comment 24

[Josh Matthews [:jdm]]

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

Comment 25

[Josh Matthews [:jdm]]

I've pushed this patch to our automated testing server. If there are no test failures related to this change, we can merge it!

Comment 26

[Amit Kr. Chandra [:wizard_A]]

Thanks a ton Josh. I hope to do better in my next bug fix.

Comment 27

[Josh Matthews [:jdm]]

The failures in on the test machines are all known Windows 7 problems that are unrelated to these changes, so this is good to go.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d93c6954d1

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Just as an fyi for the future, I had to edit the commit message to remove the ":" because the commit hook rejects "Bug: XXX", but accepts "Bug XXX".

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4d93c6954d1