Closed Bug 831107 Opened 7 years ago Closed 6 years ago

Fix up mozTCPSocket's ArrayBuffer support

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: vlad, Assigned: jdm)

References

Details

(Keywords: perf, Whiteboard: [email perf])

Attachments

(2 files, 8 obsolete files)

3.94 KB, patch
Details | Diff | Splinter Review
37.01 KB, patch
Details | Diff | Splinter Review
Bug 733573 introduced mozTCPSocket, which is quite handy, even behind a pref.  However, its ArrayBuffer support has a numeber of problems:

1) It claims ArrayBuffer support, but send() really wants a Uint8Array.  The data read handler is passed a Uint8Array.  It should be fixed so that sending works with either raw ArrayBuffers or any of the typed array types, and that receiving gives an ArrayBuffer that the user can then wrap in whatever the appropriate type is for the API.

2) send() with the Uint8Arrays is horrible -- it converts the uint8array to a string, character by character, and then sends that!  We should fix that, as the comment at http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocket.js#450 mentions
3) nsIBinaryInputStream needs an extension -- readArrayBuffer().  Right now onDataAvailable creates a Uint8Array, calls nsIBIS.readByteArray (which returns a JS array) and then calls set() with that array.  That causes a slow element-by-element copy
Assignee: nobody → josh
Perf traces showed we are wasting a lot of time in initial e-mail sync because of this bug.  We would very much like to fix this and uplift it to v1.0.1.  Requesting tef+.
blocking-b2g: --- → tef?
major email improvements are gained by this and our partners have been screaming for email performance gains.  adding this to the list of TEF+.  please prioritize this and get it fixed in the next couple of days if possible.
blocking-b2g: tef? → tef+
Does this improve email startup perf? Or post-startup perf while syncing? Or both?
Keywords: perf
Whiteboard: [email perf][status: patches in progress]
Comment on attachment 738546 [details] [diff] [review]
Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Benjamin
Attachment #738546 - Attachment is obsolete: true
Attachment #738546 - Flags: review?(benjamin)
Attachment #738546 - Flags: review?(benjamin)
(In reply to Dietrich Ayala (:dietrich) from comment #6)
> Does this improve email startup perf? Or post-startup perf while syncing? Or
> both?

Post-startup perf, unless you include online sync as part of startup.

Main-thread CPU utilization while synchronizing IMAP folders will be improved.  Since it's main thread, any wasted time affects our UI responsiveness.

This is most critical during initial sync when you first add an account where we were becoming unresponsive for ~1 second.
Status: NEW → ASSIGNED
Benjamin, can you review the new input stream that reads from an ArrayBuffer?
Attachment #738830 - Flags: review?(benjamin)
Attachment #738547 - Attachment is obsolete: true
Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

Donovan, are you good to review the TCPSocket changes?
Attachment #738830 - Flags: review?(dpreston)
Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

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

There is one unfortunate fallout of this change -- it's not possible to slice an ArrayBuffer (e.g. create a second ArrayBuffer that refers to a subregion of the original one).  ArrayBuffer.slice() copies, so that's not useful.  This can create a problem when trying to send, for example, a Uint8Array that refers to a portion of an arrayBuffer.  e.g.:

var buf = new ArrayBuffer(100);
var portion = new Uint8Array(buf, 10, 25);  // refers to the 25 bytes starting at index 10

At this point, portion.buffer === buf, which is the entire 100-byte buffer; there is no way to just pass the bytes covered by "portion" as an ArrayBuffer.

A solution is to extend the interface to allow sending an explicit offset and length, e.g.:

  socket.send(portion.buffer, portion.byteOffset, portion.byteLength);

::: dom/network/src/TCPSocket.js
@@ -94,5 @@
> -  // is no need for a wrapper to exist around the typed array from the
> -  // perspective of content.  (The wrapper is bad because it only lets content
> -  // see length, and forbids access to the array indices unless we excplicitly
> -  // list them all.)  We will then access the array through a wrapper, but
> -  // since we are chrome-privileged, this does not pose a problem.

Should we just get rid of useWin if this is no longer a problem?
Whiteboard: [email perf][status: patches in progress] → [email perf][status: needs review ben, donovan]
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> ::: dom/network/src/TCPSocket.js
> @@ -94,5 @@
> > -  // is no need for a wrapper to exist around the typed array from the
> > -  // perspective of content.  (The wrapper is bad because it only lets content
> > -  // see length, and forbids access to the array indices unless we excplicitly
> > -  // list them all.)  We will then access the array through a wrapper, but
> > -  // since we are chrome-privileged, this does not pose a problem.
> 
> Should we just get rid of useWin if this is no longer a problem?

I wanted to, but it's still used for IPC.
Attachment #738560 - Attachment description: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first. → Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.
Attachment #738830 - Attachment description: Add general ArrayBuffer support to TCPSocket. → Part 2: Add general ArrayBuffer support to TCPSocket.
Tiny update to use new when constructing the typed array.
Attachment #738993 - Flags: review?(benjamin)
Attachment #738560 - Attachment is obsolete: true
Attachment #738560 - Flags: review?(benjamin)
Attached patch Part 2 for mozilla-b2g18 (obsolete) — Splinter Review
Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

I am not a good reviewer for this code, please talk to somebody from networking.
Attachment #738830 - Flags: review?(benjamin)
Attachment #738993 - Flags: review?(benjamin)
Comment on attachment 738999 [details] [diff] [review]
Part 2 for mozilla-b2g18

I lied, this doesn't build :/
Attachment #738999 - Attachment is obsolete: true
Comment on attachment 738993 [details] [diff] [review]
Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

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

This looks totally fine, but please upload a version of the patch with the RootedObject change; too important for a local fixup.

::: xpcom/io/nsBinaryStream.cpp
@@ +721,5 @@
>  NS_IMETHODIMP
> +nsBinaryInputStream::ReadArrayBuffer(uint32_t aLength, JSContext* cx, JS::Value* _rval)
> +{
> +    JSAutoRequest ar(cx);
> +    JSObject* buffer = JS_NewArrayBuffer(cx, aLength);

Use RootedObject here, otherwise you're relying on the newly-constructed-object root to save you until you assign to _rval (which I assume is a gc root itself).  Read() might end up calling down in to JS for a JS-implemented stream though, and badness will result.
Comment on attachment 738830 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

In fact, unless there is a particular reason why ArrayBufferInputStream needs to live in XPCOM, I'd like that to live under netwerk as well.
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #18)
> Comment on attachment 738993 [details] [diff] [review]
> Part 1: Allow TCPSocket consumers to receive typed array data without
> converting it to a byte string first.
> 
> Review of attachment 738993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks totally fine, but please upload a version of the patch with the
> RootedObject change; too important for a local fixup.
> 
> ::: xpcom/io/nsBinaryStream.cpp
> @@ +721,5 @@
> >  NS_IMETHODIMP
> > +nsBinaryInputStream::ReadArrayBuffer(uint32_t aLength, JSContext* cx, JS::Value* _rval)
> > +{
> > +    JSAutoRequest ar(cx);
> > +    JSObject* buffer = JS_NewArrayBuffer(cx, aLength);
> 
> Use RootedObject here, otherwise you're relying on the
> newly-constructed-object root to save you until you assign to _rval (which I
> assume is a gc root itself).  Read() might end up calling down in to JS for
> a JS-implemented stream though, and badness will result.

The conservative stack scanner is still in effect, so I don't think this is as unsafe as you think it is. Regardless, I'll make the change.
Whiteboard: [email perf][status: needs review ben, donovan] → [email perf][status: needs review donovan]
Part 2 of this patch isn't applying cleanly for me, can you double check it? Thanks.
mozilla-b2g18 patch.
Attachment #739118 - Flags: review?(vladimir)
Attachment #738830 - Attachment is obsolete: true
Attachment #738830 - Flags: review?(dpreston)
Attachment #738993 - Attachment is obsolete: true
Attachment #739118 - Attachment description: Add general ArrayBuffer support to TCPSocket. → Part 2: Add general ArrayBuffer support to TCPSocket.
Attachment #739118 - Flags: review?(dpreston)
Comment on attachment 739119 [details] [diff] [review]
Part 1: Allow TCPSocket consumers to receive typed array data without converting it to a byte string first.

Looks good
Attachment #739119 - Flags: review?(vladimir) → review+
The API semantics I went with is that the new byteOffset and byteLength arguments are optional (and unused if the binary type is not arraybuffer), with byteLength defaulting to the length of the array buffer if it is 0.
Comment on attachment 739118 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

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

Looks good as well.  Thanks for jumping on this!

::: dom/network/src/TCPSocketChild.cpp
@@ +196,5 @@
>      if (!data) {
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
>      FallibleTArray<uint8_t> fallibleArr;
> +    if (!fallibleArr.InsertElementsAt(0, data + aByteOffset, nbytes)) {

Damn, too bad we have to copy the data here -- would be nice if we could just SendData<uint8_t[]>(data + aByteOffset, nbytes)
Attachment #739118 - Flags: review?(vladimir) → review+
Comment on attachment 739118 [details] [diff] [review]
Part 2: Add general ArrayBuffer support to TCPSocket.

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

Looks correct to me. Tests pass. Thanks for taking this!
Attachment #739118 - Flags: review?(dpreston) → review+
Attachment #739119 - Attachment is obsolete: true
Patch for mozilla-b2g18.
Attachment #739118 - Attachment is obsolete: true
Whiteboard: [email perf][status: needs review donovan] → [email perf][fixed-on-birch]
Blocks: 863664
https://hg.mozilla.org/mozilla-central/rev/6a1f26c76aec
https://hg.mozilla.org/mozilla-central/rev/3494269ba9fb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Whiteboard: [email perf][fixed-on-birch] → [email perf]
This broke the Firefox OS Simulator <https://github.com/mozilla/r2d2b2g/issues/459> and B2GRemote, which both use nsIDOMTCPSocket to communicate with FxOS devices using ADB.
Sorry, I was assured that the email app was the only client. Luckily the changes should be particularly invasive; where you previously passed Uint8Array values, you can now pass ArrayBuffer instead.
Apologies too for giving those assurances.  My mental checklist didn't include chrome-space code that I assumed would be using something more like socket.jsm.
Would it also explain total breakage of the gaia email client like this?

E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
E/GeckoConsole(  601): [JavaScript Error: "[Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (DataCloneError)"  location: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js Line: 2536"]" {file: "app://email.gaiamobile.org/js/ext/mailapi/main-frame-setup.js" line: 2536}]
Resolution: FIXED → INCOMPLETE
(In reply to Alexandre LISSY :gerard-majax from comment #37)
> Would it also explain total breakage of the gaia email client like this?

Make sure you have a version of gaia that has the updated email client. I saw the commits go through to update to the interface provided by this bug, so your gaia is probably out of date.
(In reply to Donovan Preston from comment #38)
> (In reply to Alexandre LISSY :gerard-majax from comment #37)
> > Would it also explain total breakage of the gaia email client like this?
> 
> Make sure you have a version of gaia that has the updated email client. I
> saw the commits go through to update to the interface provided by this bug,
> so your gaia is probably out of date.

Thanks but I did ensure. However, it was a real bug, see bug 864775.
Ah, I see, sorry.
(In reply to Josh Matthews [:jdm] from comment #35)
> Sorry, I was assured that the email app was the only client. Luckily the
> changes should be particularly invasive; where you previously passed
> Uint8Array values, you can now pass ArrayBuffer instead.

I'm guessing you meant they should *not* be particularly invasive.  Which makes sense.  But I'm having trouble making them anyway.  I have what I think is the right set of changes, and debug output suggests I'm sending identical bits over the wire, plus some simple transactions work; but pushing an app to a device fails.  Is there any chance I'm doing something obviously wrong?

https://github.com/mozilla/r2d2b2g/pull/465/files
Looks like sockSend needs updating, which would explain why pushing a file fails.
Resolution: INCOMPLETE → FIXED
(In reply to Myk Melez [:myk] [@mykmelez] from comment #41)
> I have what I think is the right set of changes, and debug output suggests
> I'm sending identical bits over the wire

More precisely: passing identical bits to the socket object's send() method.  Unclear whether they're still identical over the wire.


(In reply to Josh Matthews [:jdm] from comment #42)
> Looks like sockSend needs updating, which would explain why pushing a file
> fails.

It isn't obvious from the patch view, but sockSend is updated with the change to line 441:

https://github.com/mozilla/r2d2b2g/pull/465/files#L0L441

This is clearer in the view of the file with the change applied:

https://github.com/mykmelez/r2d2b2g/blob/c6c268eaa995a01606c4fbd81fb56ed9ac9fd5f2/addon/lib/adb.js#L417-L442
After further research, it turns out that TCPSocket.send() now chokes when passed an ArrayBuffer it was previously passed.  And the Simulator/B2GRemote's ADB module reuses some ArrayBuffer objects when it sends a file to a device.

It even modifies one of them in the process (a Uint32Array(1) that it uses to send several values), which I thought was perhaps the problem.  But even when it doesn't modify the object, as with its fileData array, which it sends in chunks, calling nsIDOMTCPSocket.send() twice with the same ArrayBuffer still causes the other side to get the wrong data.

Presumably the problem (or intended limitation?) is really in ArrayBufferInputStream, which TCPSocket now uses.  In any case, I updated the ADB module to always send a new ArrayBuffer, including creating one for each file chunk, and it now works with the new API.
chokes = explodes, or chokes = since it holds onto your buffers and you might mutate them after you give it to them, it sends the mutated data rather than the data you originally put in?
I thought that was the problem: the caller was mutating buffers, and TCPSocket was holding onto them and then sending the mutated data in place of the original data.  But the caller doesn't mutate its file data buffer, it just sends different chunks of it by twiddling the offset and length.  Yet that too triggers a failure.

I'm flying somewhat blind at the moment, because I don't have a view into the data the device receives, so I only know whether or not it thinks it received it successfully and installs the app I push to it.  But that was sufficient for me to determine that replacing that mutating buffer was sufficient to get things working when the caller sends the file data in one chunk; while larger files that the caller sends in multiple chunks also need unique buffers for each chunk.
I've reproduced the problem in an xpcshell test by sending the buffer from an encoded "OK" followed by some unrelated buffer followed by the first buffer again. If the duplicate buffers are uninterrupted, the server receives the data correctly. If they're sent around some other data, only the first buffer is received, with no other data to follow.
Interestingly enough, the same results occur when using a non-arraybuffer socket and sending a duplicate string value. Internally, we use nsStringInputStream to read the data, which conveniently is what I based ArrayBufferInputStream on, leading me to suspect that both implementations of Read or ReadSegments are not quite right. I've filed bug 865652 for this; let's leave this resolved bug alone now.
Depends on: 886387
Depends on: 886391
You need to log in before you can comment on or make changes to this bug.