> 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 > >
