mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 54110: Added `process::loop` abstraction.
Date Tue, 29 Nov 2016 19:50:09 GMT


> On Nov. 28, 2016, 6:47 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/loop.hpp, line 137
> > <https://reviews.apache.org/r/54110/diff/1/?file=1570957#file1570957line137>
> >
> >     Future<bool> condition = true;
> >     
> >     while (condition.get()) {
> >       Future<T> next = discard_if_necessary(iterate());
> >       if (next.isReady()) {
> >         condition = discard_if_necessary(body(next.get()));
> >         if (condition.isReady()) {
> >           continue;
> >         } else {
> >           block_on_condition();
> >           return;
> >         }
> >       } else {
> >         block_on_next();
> >         return;
> >       }
> >     }
> >     
> >     promise.set(Nothing()); // All done!
> >     
> >     
> >     
> >     void block_on_next()
> >     {
> >         next
> >           .onAny([]() {
> >               if (next.isReady()) {
> >                 condition = discard_if_necessary(body(next.get()));
> >                 if (condition.isReady()) {
> >                   run(loop);
> >                 } else {
> >                   block_on_condition();
> >                 }
> >               } else if (isDiscarded) {
> >               } else if (isFailed) {
> >               }
> >           });
> >     
> >     }
> >     
> >     
> >     void block_on_condition()
> >     {
> >           condition
> >             .onAny([]() {
> >               if (condition.isReady()) {
> >                 if (condition.get()) {
> >                   run(loop);
> >                 } else {
> >                   promise.set(Nothing());
> >                 }
> >               } else if (isDiscarded) {
> >               } else if (isFailed) {
> >               }
> >             });
> >     }

I think would suggest breaking this up into stages instead.
This approach removes duplicate code such as `body(next.get())`, handling of `condition` and
`promise.set(Nothing())`.

The transitions are: `run` --`next.isReady`--> `run_body` --`condition.isReady`--> `run_condition`.

```cpp
template <typename Iterate, typename Body, typename T>
class Loop {
public:
  void run() {
    // This is the tight loop that we ideally stay in. We leave
    // if we're gonna be blocked by `next`, or if we're done.
    for (;;) {
      next = propagate_discard(iterate());
      if (!next.isReady()) {
        break;
      }
      if (!run_body()) {
        return;
      }
    }

    // We're here because we're blocked on `next`,
    // so let's defer a continuation on it.
    next.onAny(defer(pid, [=](const Future<T>&) {
      if (next.isReady()) {
        if (run_body()) {
          run();
        }
      } else if (next.isFailed()) {
        promise.fail(next.failure());
      } else if (next.isDiscarded()) {
        promise.discard();
      }
    }));
  }
  
private:
  template <typename U>
  Future<U> propagate_discard(Future<U> future) const {
    if (promise.future().hasDiscard()) {
      future.discard();
    }
    return future;
  }
  
  bool run_body() {
    condition = propagate_discard(body(next.get()));

    // This is the short-circuit. Ideally we go through here,
    // but if the `condition` is blocked, then we proceed.
    if (condition.isReady()) {
      return run_condition();
    };

    // We're here because we're blocked on `condition`,
    // so let's defer a continuation on it.
    condition.onAny(defer(pid, [=](const Future<bool>&) {
      if (condition.isReady()) {
        if (run_condition()) {
          run();
        }
      } else if (condition.isFailed()) {
        promise.fail(next.failure());
      } else if (condition.isDiscarded()) {
        promise.discard();
      }
    }));

    return false;
  }
  
  bool run_condition() {
    bool cont = condition.get();
    if (!cont) {
      promise.set(Nothing());
    }
    return cont;
  }
  
  /* members... */
};
```


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54110/#review157168
-----------------------------------------------------------


On Nov. 28, 2016, 10:33 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54110/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2016, 10:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added `process::loop` abstraction.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/loop.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/loop_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54110/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message