forwarder2: Make the Forwarder instances operate on a single thread.

forwarder2: Make the Forwarder instances operate on a single thread.

The Forwarder class used to operate on its own internal thread. This was
leading to e.g. 256 threads being spawned since Chrome keeps a lot of sockets
around in its socket pool.

This CL makes the ForwarderManager have an internal thread performing a single
select() on all the socket file descriptors and it notifies all the Forwarder
instances when some events occur.

The fact that ForwarderManager has an internal thread that joins on destruction
also makes the interactions between ForwarderManager and its clients much
simpler. In particular it allows us to remove its ref-counted thread-safe
delegate.

This CL should make the device_forwarder daemon more lightweight and make the
results in page_cyclers hopefully more stable.

The internal buffer used to forward the traffic is also shrunk to 32 KBytes
since 128 KBytes was way too big (read() rarely returns more than 16 KBytes).

BUG=332403
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247694

[Philippe, 2014-01-27 10:31:39]

Sorry David for bothering you with forwarder2 again :) Marcus is OOO until next
week (and you are a tools/android/ owner).

This should hopefully be the last CL in forwarder2 :)

[qsr, 2014-01-27 13:28:28]

LGTM with nits.

https://codereview.chromium.org/148113003/diff/1/tools/android/forwarder2/for...
File tools/android/forwarder2/forwarder.cc (right):

https://codereview.chromium.org/148113003/diff/1/tools/android/forwarder2/for...
tools/android/forwarder2/forwarder.cc:207: char buffer_[32 * 1024];
why getting rid of the constant?

https://codereview.chromium.org/148113003/diff/80001/tools/android/forwarder2...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/80001/tools/android/forwarder2...
tools/android/forwarder2/forwarders_manager.cc:110: // Notify the Forwarder
instances and remove the ones that are closed.
Comment has been moved. Should probably be before the loop.

[Philippe, 2014-01-27 13:41:35]

Thanks Benjamin!

https://codereview.chromium.org/148113003/diff/1/tools/android/forwarder2/for...
File tools/android/forwarder2/forwarder.cc (right):

https://codereview.chromium.org/148113003/diff/1/tools/android/forwarder2/for...
tools/android/forwarder2/forwarder.cc:207: char buffer_[32 * 1024];
On 2014/01/27 13:28:29, qsr wrote:
> why getting rid of the constant?

I mainly wanted to get rid of the comment but I added the constant back :)

https://codereview.chromium.org/148113003/diff/80001/tools/android/forwarder2...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/80001/tools/android/forwarder2...
tools/android/forwarder2/forwarders_manager.cc:110: // Notify the Forwarder
instances and remove the ones that are closed.
On 2014/01/27 13:28:29, qsr wrote:
> Comment has been moved. Should probably be before the loop.

Done.

[pasko, 2014-01-27 16:59:01]

lgtm

Though I looked at forwarder2 code the first time, learned much more than I can
return back. Sorry.

I could even manage to provide one style comment, which is not mandatory to
address, enjoy.

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.cc:122: if (!forwarder->IsClosed())
{
I think if you replace one loop with three, it would get easier to read (LOL),
since the reader now has to keep in mind that Shutdown() makes the forwarder
closed. Also no modifying of the index variable in the loop.

proposed pseudocode:

for (size_t i = 0; i < forwarders_.size(); ) {
  forwarders_[i]->ProcessEvents(read_fds, write_fds);
}
if (must_shutdown) {
  for (auto forwarder : forwarders_) {
    forwarder->Shutdown();
  }
  forwarders_.clear();
} else {
  // Remove forwarders that are closed.
}

This may change the order of events, so I am not even sure it would work. So
even though it repeats looping three times, the code looks more linear and more
obvious. The code would not be much longer.

Up to you.

[Philippe, 2014-01-28 08:59:09]

Adding Marcus since he's Out Of the London Office and not Out Of the Office as I
misunderstood (unless he spends his vacations in MTV :)).

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.cc:122: if (!forwarder->IsClosed())
{
On 2014/01/27 16:59:02, pasko wrote:
> I think if you replace one loop with three, it would get easier to read (LOL),
> since the reader now has to keep in mind that Shutdown() makes the forwarder
> closed. Also no modifying of the index variable in the loop.
> 
> proposed pseudocode:
> 
> for (size_t i = 0; i < forwarders_.size(); ) {
>   forwarders_[i]->ProcessEvents(read_fds, write_fds);
> }
> if (must_shutdown) {
>   for (auto forwarder : forwarders_) {
>     forwarder->Shutdown();
>   }
>   forwarders_.clear();
> } else {
>   // Remove forwarders that are closed.
> }
> 
> This may change the order of events, so I am not even sure it would work. So
> even though it repeats looping three times, the code looks more linear and
more
> obvious. The code would not be much longer.
> 
> Up to you.

You slightly cheated by using a foreach loop :) So the code would be even
longer.

The thing which is non-obvious here too is the fact that a Forwarder instance is
not necessarily immediately closed after Shutdown() was called therefore we
can't call |forwarders_.clear| as you did. If one of the internal Forwarder's
BufferCopier still has data in its buffer then it would go to the 'CLOSING'
state (as opposed to 'CLOSED') after a call to Shutdown() in which it flushes
its buffer.

So the code would rather be:
for (ScopedVector<Forwarder>::iterator it = forwarders_.begin();
         it != forwarders_.end(); ++it) {
  (*it)->ProcessEvents(read_fds, write_fds);
} 

if (must_shutdown) {
  for (ScopedVector<Forwarder>::iterator it = forwarders_.begin();
         it != forwarders_.end(); ++it) {
    (*it)->Shutdown();
  } 
}

for (size_t i = 0; i < forwarders_.size(); ) {
  Forwarder* const forwarder = forwarders_[i];
  if (!forwarder->IsClosed()) {
    ++i;
    continue;
  }
  std::swap(forwarders_[i], forwarders_.back());
  forwarders_.resize();
}
  
Which is much shorter, right? :)

It's a shame BTW that base::Callback doesn't provide an operator() so that we
can use it with STL algorithms.

[pasko, 2014-01-28 10:48:55]

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.cc:122: if (!forwarder->IsClosed())
{
On 2014/01/28 08:59:09, Philippe wrote:
> On 2014/01/27 16:59:02, pasko wrote:
> > I think if you replace one loop with three, it would get easier to read
(LOL),
> > since the reader now has to keep in mind that Shutdown() makes the forwarder
> > closed. Also no modifying of the index variable in the loop.
> > 
> > proposed pseudocode:
> > 
> > for (size_t i = 0; i < forwarders_.size(); ) {
> >   forwarders_[i]->ProcessEvents(read_fds, write_fds);
> > }
> > if (must_shutdown) {
> >   for (auto forwarder : forwarders_) {
> >     forwarder->Shutdown();
> >   }
> >   forwarders_.clear();
> > } else {
> >   // Remove forwarders that are closed.
> > }
> > 
> > This may change the order of events, so I am not even sure it would work. So
> > even though it repeats looping three times, the code looks more linear and
> more
> > obvious. The code would not be much longer.
> > 
> > Up to you.
> 
> You slightly cheated by using a foreach loop :) So the code would be even
> longer.
> 
> The thing which is non-obvious here too is the fact that a Forwarder instance
is
> not necessarily immediately closed after Shutdown() was called therefore we
> can't call |forwarders_.clear| as you did. If one of the internal Forwarder's
> BufferCopier still has data in its buffer then it would go to the 'CLOSING'
> state (as opposed to 'CLOSED') after a call to Shutdown() in which it flushes
> its buffer.

Oh, I did not realize that. It can get straight to closed or to closing.
Wonderful.
Thanks for explanation.
 
> So the code would rather be:
> for (ScopedVector<Forwarder>::iterator it = forwarders_.begin();
>          it != forwarders_.end(); ++it) {
>   (*it)->ProcessEvents(read_fds, write_fds);
> } 
> 
> if (must_shutdown) {
>   for (ScopedVector<Forwarder>::iterator it = forwarders_.begin();
>          it != forwarders_.end(); ++it) {
>     (*it)->Shutdown();
>   } 
> }
> 
> for (size_t i = 0; i < forwarders_.size(); ) {
>   Forwarder* const forwarder = forwarders_[i];
>   if (!forwarder->IsClosed()) {
>     ++i;
>     continue;
>   }
>   std::swap(forwarders_[i], forwarders_.back());
>   forwarders_.pop_back();
> }
>   
> Which is much shorter, right? :)

absolutely)

To me the most irritating was understanding how the index variable gets modified
in the loop and why we are not using end(), and such, so a more compact loop
helps here, but making process and shutdown loops separate does not in fact make
sense if we cannot do clear(). How about this:

====================
for (ScopedVector<Forwarder>::iterator it = forwarders_.begin();
     it != forwarders_.end(); ++it) {
  Forwarder* forwarder = (*it);
  forwarder->ProcessEvents(read_fds, write_fds);
  if (must_shutdown)
    forwarder->Shutdown();
}

for (size_t i = 0; i < forwarders_.size(); ) {
  Forwarder* const forwarder = forwarders_[i];
  if (!forwarder->IsClosed()) {
    ++i;
    continue;
  }
  std::swap(forwarders_[i], forwarders_.back());
  forwarders_.pop_back();
}
====================

This is roughly +3 lines, but is easier for me to read. I won't insist though.
It is already taking too much time.

> It's a shame BTW that base::Callback doesn't provide an operator() so that we
> can use it with STL algorithms.

+1, though imagine you give this freedom to certain colleagues, they'd say it's
only a half solution, instead we should switch directly to LISP.

[digit1, 2014-01-28 17:28:32]

lgtm - I like it :-)

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.cc:103: base::ScopedClosureRunner
wait_for_events_soon(
Can you avoid using this? a base::Bind() is pretty heavy and allocates heap
memory on each loop iteration (which gets released when the function returns).
This looks like un-needed CPU/heap churn to me.

Also you're binding to a function that will do its own base::Bind() to re-run
the current function, this is also very heavy in each loop iteration. I wonder
if a simple loop here would not be more appropriate, with a different way to
communicate additions / deletions to the thread.

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.h (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.h:18: class ForwardersManager {
what, no doc for this class?. Come on, you can do better than that ;-)

[qsr, 2014-01-29 09:30:17]

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.cc (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.cc:103: base::ScopedClosureRunner
wait_for_events_soon(
On 2014/01/28 17:28:33, digit1 wrote:
> Can you avoid using this? a base::Bind() is pretty heavy and allocates heap
> memory on each loop iteration (which gets released when the function returns).
> This looks like un-needed CPU/heap churn to me.
> 
> Also you're binding to a function that will do its own base::Bind() to re-run
> the current function, this is also very heavy in each loop iteration. I wonder
> if a simple loop here would not be more appropriate, with a different way to
> communicate additions / deletions to the thread.

 Given that this is only called when select returns and so, before some I/O,
this is far from being a busy loop. Starting to use a custom thread and custom
message passing for this seem to be a lot of effort for a gain that is not
really evident at this point.

[Philippe, 2014-01-29 10:50:03]

Thanks guys!

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
File tools/android/forwarder2/forwarders_manager.h (right):

https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
tools/android/forwarder2/forwarders_manager.h:18: class ForwardersManager {
On 2014/01/28 17:28:33, digit1 wrote:
> what, no doc for this class?. Come on, you can do better than that ;-)

Yeah, I kept the comment quite minimalist though :)

[commit-bot: I haz the power, 2014-01-29 10:50:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/148113003/350001

[commit-bot: I haz the power, 2014-01-29 16:15:27]

Change committed as 247694

[digit1, 2014-01-30 18:19:11]

On 2014/01/29 09:30:17, qsr wrote:
>
https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
> File tools/android/forwarder2/forwarders_manager.cc (right):
> 
>
https://codereview.chromium.org/148113003/diff/140002/tools/android/forwarder...
> 
>  Given that this is only called when select returns and so, before some I/O,
> this is far from being a busy loop. Starting to use a custom thread and custom
> message passing for this seem to be a lot of effort for a gain that is not
> really evident at this point.

This is going to happen anytime you're going to either read or write a packet,
which can be frequent when transferring large amounts of data. I don't think it
will impact performance much, but I fear this may slightly increase variability
of the results though. Especially since this is used both on the device and on
the host at the same time during normal operations.

However, this is your code now, please do as you wish, but keep this in mind in
case you see a real effect on measurements ;-)