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