mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ilya Pronin <ipro...@twopensource.com>
Subject Re: Review Request 73136: Added support for multiple event loop / IO threads for libev.
Date Fri, 22 Jan 2021 19:16:16 GMT


> On Jan. 19, 2021, 4:17 p.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?
> 
> Benjamin Mahler wrote:
>     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.

I was thinking about dropping the pointer:
```c++
extern std::vector<struct ev_loop*> loops;
```
I agree that a plain array is simpler if we want to keep it.


> On Jan. 19, 2021, 4:17 p.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?
> 
> Benjamin Mahler wrote:
>     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..?).

Makes sense! I was mostly thinking about the case where the operator does not provide `LIBPROCESS_NUM_WORKER_THREADS`
and it is defaulted to `os::cpus()`.


- Ilya


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


On Jan. 21, 2021, 10:51 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73136/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2021, 10:51 a.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/3/
> 
> 
> 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