[fuchsia] Replace use of epoll with mx_ primitives in process_fuchsia

[fuchsia] Replace use of epoll with mx_ primitives in process_fuchsia

process_fuchsia.cc has the ability to drain stdout, stderr, and exit
status from a child process. It was using epoll to multiplex these
streams which worked but required complicated emulation in libc which
we'd like to unwind as well as some tricky locking. This replaces the
epoll with a direct mx_object_wait_many() call that waits on the
Magenta handles backing the streams.

R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/d32e55e17b385c83f17604d6a15f9932c69daf42

[jamesr1, 2017-05-25 23:49:21]

jamesr@google.com changed reviewers:
+ kulakowski@google.com, zra@google.com

[jamesr1, 2017-05-25 23:49:21]

Zach - I've tested this by running 'hello_fuchsia.dart'.  I left the usleep() in
place, although I don't fully understand why this was needed since this function
appears to be blocking.  Do you know how to reproduce the problem that the
usleep was addressing in the previous code?

[zra, 2017-05-26 16:13:22]

lgtm w/ nits

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia.cc
File runtime/bin/process_fuchsia.cc (left):

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:428: 
Dart VM style is two newlines between top-level functions in .cc files.

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia.cc
File runtime/bin/process_fuchsia.cc (right):

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:469: if (mxio_ == nullptr) {
To be consistent with the rest of the VM, please use NULL instead of nullptr.

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:488: if (mxio_)
if (mxio_ != NULL)

Also, please use curly braces around single line if.

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:495: };
Maybe DISALLOW_COPY_AND_ASSIGN(MxioWaitEntry) or DISALLOW_ALLOCATION().

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:516: mx_wait_item_t wait_items[3];
const intptr_t kWaitItemsCount = 3;

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:527: usleep(5000);
This was copy-pasted from the eventhandler, but probably wasn't needed here in
the first place since the read()s from the ready fds were happening on the same
thread. This is safe to remove.

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:739: #define CHECK_FOR_ERROR(status, msg)        
                                 \
It looks like the escaped newlines were already in the right place.

[jamesr1, 2017-05-27 00:14:10]

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia.cc
File runtime/bin/process_fuchsia.cc (left):

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:428: 
On 2017/05/26 at 16:13:22, zra wrote:
> Dart VM style is two newlines between top-level functions in .cc files.

Turns out I wasn't reading your .clang-format file, fixed!

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia.cc
File runtime/bin/process_fuchsia.cc (right):

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:469: if (mxio_ == nullptr) {
On 2017/05/26 at 16:13:22, zra wrote:
> To be consistent with the rest of the VM, please use NULL instead of nullptr.

done

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:488: if (mxio_)
On 2017/05/26 at 16:13:22, zra wrote:
> if (mxio_ != NULL)
> 
> Also, please use curly braces around single line if.

done

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:495: };
On 2017/05/26 at 16:13:22, zra wrote:
> Maybe DISALLOW_COPY_AND_ASSIGN(MxioWaitEntry) or DISALLOW_ALLOCATION().

done

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:516: mx_wait_item_t wait_items[3];
On 2017/05/26 at 16:13:22, zra wrote:
> const intptr_t kWaitItemsCount = 3;

done

https://codereview.chromium.org/2903373003/diff/1/runtime/bin/process_fuchsia...
runtime/bin/process_fuchsia.cc:527: usleep(5000);
On 2017/05/26 at 16:13:22, zra wrote:
> This was copy-pasted from the eventhandler, but probably wasn't needed here in
the first place since the read()s from the ready fds were happening on the same
thread. This is safe to remove.

done

[jamesr1, 2017-05-27 00:21:39]

Feedback addressed in PS3.  Making MxioWaitEntry noncopyable makes a lot of
sense, but required tweaking the initialization flow slightly.

George - this look right to you?

[kulakowski1, 2017-05-30 20:47:55]

You're holding wait_begin/end and the mxio stuff correctly.

Just the one comment inline.

https://codereview.chromium.org/2903373003/diff/40001/runtime/bin/process_fuc...
File runtime/bin/process_fuchsia.cc (right):

https://codereview.chromium.org/2903373003/diff/40001/runtime/bin/process_fuc...
runtime/bin/process_fuchsia.cc:556: return ProcessWaitCleanup(err, err,
exit_event);
The change from ProcessWaitCleanup(out, err, exit_event) to
ProcessWaitCleanup(err, err, exit_event) looks spurious.

[jamesr1, 2017-05-30 20:53:59]

https://codereview.chromium.org/2903373003/diff/40001/runtime/bin/process_fuc...
File runtime/bin/process_fuchsia.cc (right):

https://codereview.chromium.org/2903373003/diff/40001/runtime/bin/process_fuc...
runtime/bin/process_fuchsia.cc:556: return ProcessWaitCleanup(err, err,
exit_event);
On 2017/05/30 at 20:47:55, kulakowski1 wrote:
> The change from ProcessWaitCleanup(out, err, exit_event) to
ProcessWaitCleanup(err, err, exit_event) looks spurious.

Yup, that's a typo.  Fixed!

[jamesr1, 2017-05-30 20:59:13]

Description was changed from

==========
[fuchsia] Replace use of epoll with mx_ primitives in process_fuchsia

process_fuchsia.cc has the ability to drain stdout, stderr, and exit
status from a child process. It was using epoll to multiplex these
streams which worked but required complicated emulation in libc which
we'd like to unwind as well as some tricky locking. This replaces the
epoll with a direct mx_object_wait_many() call that waits on the
Magenta handles backing the streams.
==========

to

==========
[fuchsia] Replace use of epoll with mx_ primitives in process_fuchsia

process_fuchsia.cc has the ability to drain stdout, stderr, and exit
status from a child process. It was using epoll to multiplex these
streams which worked but required complicated emulation in libc which
we'd like to unwind as well as some tricky locking. This replaces the
epoll with a direct mx_object_wait_many() call that waits on the
Magenta handles backing the streams.

R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d32e55e17b385c83f17604d6a15f9932c69daf42
==========

[jamesr1, 2017-05-30 20:59:14]

Committed patchset #3 (id:40001) manually as
d32e55e17b385c83f17604d6a15f9932c69daf42 (presubmit successful).