1353475 - PParentToChildStream and PChildToParentStream should send data only when the inputStream is read.

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Currently, the sending of the data starts immediately. We should optimize it and send data only when the first read occurs.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: ipcBlob_4_start.patch

Comment 2

[Andrea Marchesini [:baku]]

Ben, I would like you to take a look at this patch. When you have time, of course. Thanks.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Andrea, I'm quite worried about doing this without any talos tests measuring the impact.  Yes, this allows an optimization when you might pass the stream back to the original source and it could be removed, but it can also have negative performance consequences.

My guess is that many (most?) stream uses currently are going to be eagerly reading.  Adding a complete block on sending data until the other side signals the read start is going to create a stall.  Maybe this is ok, but it feels like it could have a negative perf effect for many common uses.

Is this addressing a measurable problem you are seeing or are you just trying to speculatively cut down on copying?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

For example, I'd be happy to have a windowing mechanism that provides back pressure, but that would allow some reading to occur before stalling.  I'm really worried about putting complete stalls in this path.

I used to be really concerned with copying data from child->parent->child, but I just haven't actually seen it show up as a real problem.  I'm hesitant to rik potentially creating other problems to try to solve this copying prematurely.

Comment 5

[Andrea Marchesini [:baku]]

> My guess is that many (most?) stream uses currently are going to be eagerly
> reading.  Adding a complete block on sending data until the other side
> signals the read start is going to create a stall.  Maybe this is ok, but it
> feels like it could have a negative perf effect for many common uses.

Well, in multi-e10s, just doing |URL.createObjectURL(new Blob([a long string here]);| causes the broadcasting of the BlobImpl (and its stream) to each process. That inputStream on average is not used.

Here we create a ChildToParent -> ParentToChild (for each other process).

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Can you make this an optional thing that the blob URL stuff opts-in to, but other consumers get eager reading by default?

Comment 7

[Andrea Marchesini [:baku]]

Sure!

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 1 - RemoteInputStream IPC

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - DelayStart

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I'm ok with this in principle, but is there any chance you could ask Olli or someone else for review?  I'm sorry, but I'm still a bit under the gun on this deadline for service workers.  I'm not sure I have the time to commit to reviewing this that it deserves.

Comment 11

[Andrea Marchesini [:baku]]

Smaug, can you please review these 2 patches? You have followed the implementation of these classes.
Note that the first implementation was exposing this delayStart as default. Ben suggested to have it optional.

The reason why this is needed is because, when we broadcast BlobURL, we send a copy of its nsIInputStream to each process but rarely each on of them will actually use the stream.

Interesting point: having the wrapper, means that we can avoid to expose a partial nsISeekableStream interface (see bug 1349570). Plus, we can return a valid Available() data in case we need it - this is not implemented yet because, so far, this is not needed.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8855831 [details] [diff] [review]
part 2 - DelayStart


>+// ----------------------------------------------------------------------------
>+// IPCStreamDestination::Wrapper
>+
>+class IPCStreamDestination::Wrapper final : public nsIAsyncInputStream
>+                                          , public nsISearchableInputStream
>+                                          , public nsICloneableInputStream
>+                                          , public nsIBufferedInputStream
>+{
This needs some good documentation. What is the Wrapper for? Maybe rename it to indicate how it is used.

>-  // If the source stream was taken to be sent to the other side, then we need
>-  // to start it before forgetting about it.
>-  source->Start();
>+  if (!aDelayedStart) {
>+    // If the source stream was taken to be sent to the other side, then we need
>+    // to start it before forgetting about it.
>+    source->Start();
>+  }
The comment is now very unclear. It hints that one really must call Start()... but then, we don't call it after all when
aDelayedStart is true. Please clarify.


>+  // The actor needs to receive data. This method is called only if this actor
>+  // has been created with a start delayable.
'with delayed start' (or something like that)?

> parent:
>+  // The actor needs to receive data. This method is called only if this actor
>+  // has been created with a start delayable.
ditto


This needs some comments. I'd like to read those, so r-

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - DelayStart

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Have you tested the lazy reading setup in case nothing is actually ever read?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8858253 [details] [diff] [review]
part 2 - DelayStart

 
> struct IPCRemoteStream
> {
>+  // If this is true, the stream will send data only when the first operation
>+  // is done on the destination side. The benefit of this is that we do not
>+  // send data if not needed. On the otherside, it could have a performance
otherside or other hand 

> IPCStreamDestination::TakeReader()
> {
>   MOZ_ASSERT(mReader);
>+  MOZ_ASSERT(!mDelayedStartInputStream);
>+
>+  if (mDelayedStart) {
>+    mDelayedStartInputStream =
>+      new DelayedStartInputStream(this, mReader.forget());
>+    RefPtr<nsIAsyncInputStream> inputStream = mDelayedStartInputStream;
>+    return inputStream.forget();
>+  }
I don't understand this setup. What is the lifetime management for mDelayedStartInputStream?
It is not documented in IPCStreamDestination.h


>+IPCStreamDestination::DispatchRunnable(already_AddRefed<nsIRunnable>&& aRunnable)
>+{
>+  nsCOMPtr<nsIRunnable> runnable = aRunnable;
>+  mOwningThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
Why can't you just dispatch aRunnable to Dispatch (possibly by using Move())?



IPCStreamDestination::mDelayedStartInputStream needs some clarification, something that it is needed so that
IPCStreamDestination::ActorDestroyed() can call its DestinationShutdown


This needs some tests. Like object url usage so that data is read from such url and tests where data isn't read. Please don't land without some explicit tests exercising the relevant code paths.
Or explain which tests do test this all.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(if we need other patches to actually use this stuff, landing this should wait for those.)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab72661ee46
Implement AutoIPCStream with delayed start - part 1 - changing the IPDL dictionaries for streams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ccae2720f05
Implement AutoIPCStream with delayed start - part 2 - delayed start, r=smaug

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1353629 and bug 1353475:

Bug 1353475:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b728b818169c35e1dc65b71d424e9542abb52b0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e574e7afdf5fbdb5620c483164e997c1ea4d959f

Bug 1353629:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8a7f3baf26ff74d94ec1eec64d04b8051a3d41e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67dc5007a113d3eeaf6d4fe2e85bbe1a9a4533a
https://hg.mozilla.org/integration/mozilla-inbound/rev/802cbff80adaa3051cf40621099887523a27c439
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab390bce545cceed9f0faed321427d014bcead1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca68d3ea9eb9a53133c13bf40fa8e120737c6788
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbd6a09cdeef9fd9df5dcdf40a65d459681e57c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f58b24833af45056c7d54bc8fd7f234f903da55
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69c86f4235ebc6bef174baebe2cf940ad275b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/76e6ae0b86fb49653ee65a9b1b8baffc20e13f62
https://hg.mozilla.org/integration/mozilla-inbound/rev/564bd22ce8898109f80ef3b421e9c5329cc86dc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cabdccfa10f0ed0a934b395c60408ed5de6d2306
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb12febc369226d617febbf08af6b4178ce46f29
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c48858c3812645a72ed9896202a41849281666c
https://hg.mozilla.org/integration/mozilla-inbound/rev/86f4670afa32d1d5f7e759295c718277d81dc03d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e8c3d77689601007dff98d61b3149d3f461660

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d4ab7cc36551d885b6640e180ead7093bf610a4c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=93042867&repo=mozilla-inbound

08:30:25     INFO - TEST-START | dom/filesystem/compat/tests/test_formSubmission.html
08:30:25     INFO - GECKO(5220) | Unable to read VR Path Registry from C:\Users\cltbld\AppData\Local\openvr\openvrpaths.vrpath
08:30:25     INFO - GECKO(5220) | JavaScript error: http://mochi.test:8888/tests/dom/filesystem/compat/tests/script_entries.js, line 12: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]
08:35:53     INFO - TEST-INFO | started process screenshot
08:35:53     INFO - TEST-INFO | screenshot: exit 0
08:35:53     INFO - TEST-UNEXPECTED-FAIL | dom/filesystem/compat/tests/test_formSubmission.html | Test timed out. 
08:35:53     INFO -     reportError@SimpleTest/TestRunner.js:121:7
08:35:53     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:35:53     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
08:35:53     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
08:35:53     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
08:35:53     INFO -     hookupTests@SimpleTest/setup.js:266:5
08:35:53     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
08:35:53     INFO -     hookup@SimpleTest/setup.js:246:5
08:35:53     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Ccltbld%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1

08:35:54     INFO - TEST-START | dom/filesystem/compat/tests/test_no_dnd.html
08:35:54     INFO - GECKO(5220) | JavaScript error: http://mochi.test:8888/tests/dom/filesystem/compat/tests/script_entries.js, line 12: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]
08:36:17     INFO -  JavaScript error: resource://gre/modules/FormHistory.jsm, line 373: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
08:36:17     INFO -  JavaScript error: resource://gre/modules/PlacesUtils.jsm, line 1988: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]
08:41:23     INFO - Not taking screenshot here: see the one that was previously logged
08:41:23     INFO - TEST-UNEXPECTED-FAIL | dom/filesystem/compat/tests/test_no_dnd.html | Test timed out. 
08:41:23     INFO -     reportError@SimpleTest/TestRunner.js:121:7
08:41:23     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:41:23     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
08:41:23     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
08:41:23     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
08:41:23     INFO -     hookupTests@SimpleTest/setup.js:266:5
08:41:23     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
08:41:23     INFO -     hookup@SimpleTest/setup.js:246:5
08:41:23     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Ccltbld%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1

Comment 19

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

I want to pin the test_buffered failures on these patches, too: https://treeherder.mozilla.org/logviewer.html#?job_id=93045728&repo=mozilla-inbound

Retriggered a bunch to see where those started: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&noautoclassify&group_state=expanded&fromchange=e2ea72d6ed2ca90f8ae3206d3f7c177031b6bdd1&filter-searchStr=c2aa52716a4dd0c957b2c89296a908823b2b47ca&selectedJob=93074139

Comment 20

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

(In reply to Wes Kocher (:KWierso) from comment #19)
> I want to pin the test_buffered failures on these patches, too:
> https://treeherder.mozilla.org/logviewer.html#?job_id=93045728&repo=mozilla-
> inbound
> 
> Retriggered a bunch to see where those started:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&noautoclassify&group_state=expanded&fromchange=e2ea72d6ed2ca90f8ae320
> 6d3f7c177031b6bdd1&filter-
> searchStr=c2aa52716a4dd0c957b2c89296a908823b2b47ca&selectedJob=93074139

Looks like it did indeed cause those failures.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f3a2c33c8f
Implement AutoIPCStream with delayed start - part 1 - changing the IPDL dictionaries for streams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/322035f4e783
Implement AutoIPCStream with delayed start - part 2 - delayed start, r=smaug

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/f6f3a2c33c8f
https://hg.mozilla.org/mozilla-central/rev/322035f4e783