mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 73136: Added support for multiple event loop / IO threads for libev.
Date Wed, 20 Jan 2021 21:30:03 GMT


> On Jan. 20, 2021, 12:17 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.hpp
> > Line 30 (original), 30 (patched)
> > <https://reviews.apache.org/r/73136/diff/1/?file=2244173#file2244173line30>
> >
> >     Why don't we use a `std::vector` here?

Yeah I also considered this here (and for the watchers, functions / functions mutex, since
I think they should be consistent):

```
// Declaration / Initialization
extern std::vector<struct ev_loop*>* loops;
loops = new vector<struct ev_loop*>();

(*loops)[index]; // Or:
loops->at(index);

delete loops;
```

vs

```
extern struct ev_loop** loops;
loops = new struct ev_loop*[num_loops];

loops[index];

delete[] loops;
```

The above vector code seemed a bit more complicated to me given the indexing difference, and
it's worse for the others I think, e.g.:

```
extern std::vector<std::queue<lambda::function<void()>>>* functions;

extern std::queue<lambda::function<void()>>* functions;
```

Since there's now a pointer and a vector (instead of just a pointer), it seemed at first glance
more complicated. Granted, with the pointer you have to know that it's an array whereas the
vector is more explicit about that.

I don't have a strong opinion here, let me know if you think it's better if they're all vectors.


> On Jan. 20, 2021, 12:17 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.hpp
> > Lines 46-49 (patched)
> > <https://reviews.apache.org/r/73136/diff/1/?file=2244173#file2244173line46>
> >
> >     Should `get_loop` be a part of `LoopIndex` construction to avoid being overlooked?

Hm.. are you suggesting something like this?

```
run_in_event_loop(
    LoopIndex(fd),
    some_func);
```

That seems not very readable / intuitive to me vs calling get_loop. If we're worried about
someone constructing a LoopIndex on their own with some index, without calling get_loop, then
we could make the constructor private. But, since this is such a narrowly consumed API, I'm
not too worried about that. My thought was that someone wants to call `run_in_event_loop`,
and they see they need to pass in a `LoopIndex`. When they look that up they see that they
need to call `get_loop` to get one, and it seems unlikely they'll do something wrong instead.


> On Jan. 20, 2021, 12:17 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/src/posix/libev/libev.cpp
> > Lines 185-186 (original), 257-258 (patched)
> > <https://reviews.apache.org/r/73136/diff/1/?file=2244174#file2244174line259>
> >
> >     Should we reduce the number of libprocess actor threads by the number of additional
IO threads we spawn here?

This brings up a good point to think about. I think we should just let users configure the
two (worker threads and io threads) on their own, otherwise it will be surprising when the
number you specify is not what is used (I said 12 worker threads but there are only 8..?).


- Benjamin


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


On Jan. 12, 2021, 7:22 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73136/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2021, 7:22 p.m.)
> 
> 
> Review request for mesos and Ilya Pronin.
> 
> 
> Bugs: MESOS-10208
>     https://issues.apache.org/jira/browse/MESOS-10208
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current approach to I/O in libprocess, with a single thread
> performing all of the the I/O polling and I/O syscalls, cannot keep
> up with the I/O load on massive scale mesos clusters (which use
> libev rather than libevent).
> 
> This adds support via a LIBPROCESS_LIBEV_NUM_IO_THREADS env variable
> for configuring the number of threads running libev event loops,
> which allows users to spread the IO load across multiple threads.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/posix/libev/libev.hpp d451931871db650894e4a6e5b0d19ba876f65391

>   3rdparty/libprocess/src/posix/libev/libev.cpp b38e7a0f882a8c24950bdc6fd74a4d25fc68549e

>   3rdparty/libprocess/src/posix/libev/libev_poll.cpp 96913a65507ca3540066e28448684d1e3fa540ca

> 
> 
> Diff: https://reviews.apache.org/r/73136/diff/1/
> 
> 
> Testing
> -------
> 
> So far only manual testing:
> 
> ```
> # Error
> $ make check -j16 TEST_DRIVER="" GTEST_FILTER="-ProcessRemoteLinkTest.RemoteLinkLeak"
LIBPROCESS_LIBEV_NUM_IO_THREADS=0
> $ make check -j16 TEST_DRIVER="" GTEST_FILTER="-ProcessRemoteLinkTest.RemoteLinkLeak"
LIBPROCESS_LIBEV_NUM_IO_THREADS=1025
> 
> # Success
> $ make check -j16 TEST_DRIVER="" GTEST_FILTER="-ProcessRemoteLinkTest.RemoteLinkLeak"
LIBPROCESS_LIBEV_NUM_IO_THREADS=1
> $ make check -j16 TEST_DRIVER="" GTEST_FILTER="-ProcessRemoteLinkTest.RemoteLinkLeak"
LIBPROCESS_LIBEV_NUM_IO_THREADS=32
> ```
> 
> Will follow up with some test(s) that leverage the reinitialize support in libprocess,
so that the testing doesn't need to be done manually.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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