----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50759/#review144631 -----------------------------------------------------------
This looks good to me - I've made some (I hope) constructive suggestions. I'm having a hard time understanding this code as it stands (and I originally wrote it!) so I've tried to simplify your change so that it doesn't introduce extra cases to consider. src/qpid/sys/epoll/EpollPoller.cpp (line 551) <https://reviews.apache.org/r/50759/#comment210655> You don't need to mention Jiras in code comments. You can get the issue number from the source control system and the source control change from the issue record. So adding the issue number in the code is unnecessary (and just clutter IMO) src/qpid/sys/epoll/EpollPoller.cpp (line 554) <https://reviews.apache.org/r/50759/#comment210667> See replacement suggestion below. src/qpid/sys/epoll/EpollPoller.cpp (line 568) <https://reviews.apache.org/r/50759/#comment210666> See replacement suggestion below. src/qpid/sys/epoll/EpollPoller.cpp (line 661) <https://reviews.apache.org/r/50759/#comment210665> This should really be if (timeout==TIME_INFINITE) because now timeoutMs can never be -1 src/qpid/sys/epoll/EpollPoller.cpp (line 669) <https://reviews.apache.org/r/50759/#comment210664> I think it would be easier to understand all the timer manipulations to put something like: timeoutMs = std::min(Duration(now(), targetTimeout)/TIME_MSEC, maxEpollWait); [Or you could make maxEpollWait a Duration which might be neater and do: timeoutMs = std::min(Duration(now(), targetTimeout), maxEpollWait) / TIME_MSEC; which I think should work because of the implicit conversion from Duration->int64_t.] at the top of the loop and remove the recalculation from here. - Andrew Stitcher On Aug. 3, 2016, 5:43 p.m., Cliff Jansen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50759/ > ----------------------------------------------------------- > > (Updated Aug. 3, 2016, 5:43 p.m.) > > > Review request for qpid and Andrew Stitcher. > > > Bugs: qpid-7373 > https://issues.apache.org/jira/browse/qpid-7373 > > > Repository: qpid-cpp > > > Description > ------- > > If a C++ broker is lightly loaded with many short lived connections such that > at least one worker thread remains unwoken from the epoll_wait, its > DeletionManager::ThreadStatus::handles grows without bound and the associated > handles retain a shared_ptr ref and never go away. > > This patch allows IO threads a maximum of one minute to accumulate > DeletionManager shared pointer references before resuming the wait loop which > involves calling > > PollerHandleDeletionManager.markAllUnusedInThisThread(); > > The overhead is small on brokers light thread load and non-existent on busy > brokers. > > > Diffs > ----- > > src/qpid/sys/epoll/EpollPoller.cpp 6fdf996 > > Diff: https://reviews.apache.org/r/50759/diff/ > > > Testing > ------- > > Jira test case, PollerTest.cpp > > > Thanks, > > Cliff Jansen > >
