diff --git a/src/dmclock_server.h b/src/dmclock_server.h index 197f852..6fc80bc 100644 --- a/src/dmclock_server.h +++ b/src/dmclock_server.h @@ -331,16 +331,13 @@ namespace crimson { // NB: because a deque is the underlying structure, this // operation might be expensive - template - bool remove_by_req_filter_forwards(std::function filter, - Collect& out) { + bool remove_by_req_filter_fw(std::function filter_accum) { bool any_removed = false; for (auto i = requests.begin(); i != requests.end(); /* no inc */) { - if (filter(*i->request)) { + if (filter_accum(*i->request)) { any_removed = true; - out.push_back(*i->request); i = requests.erase(i); } else { ++i; @@ -351,31 +348,28 @@ namespace crimson { // NB: because a deque is the underlying structure, this // operation might be expensive - template - bool remove_by_req_filter_backwards(std::function filter, - Collect& out) { + bool remove_by_req_filter_bw(std::function filter_accum) { bool any_removed = false; - for (auto i = --requests.end(); - /* no cond */; - --i) { - if (filter(*i->request)) { + for (auto i = requests.rbegin(); + i != requests.rend(); + /* no inc */) { + if (filter_accum(*i->request)) { any_removed = true; - out.push_back(*i->request); - i = requests.erase(i); + i = decltype(i){ requests.erase(std::next(i).base()) }; + } else { + ++i; } - if (requests.begin() == i) break; } return any_removed; } - template - inline bool remove_by_req_filter(std::function filter, - Collect& out, - bool visit_backwards) { + inline bool + remove_by_req_filter(std::function filter_accum, + bool visit_backwards) { if (visit_backwards) { - return remove_by_req_filter_backwards(filter, out); + return remove_by_req_filter_bw(filter_accum); } else { - return remove_by_req_filter_forwards(filter, out); + return remove_by_req_filter_fw(filter_accum); } } @@ -438,25 +432,13 @@ namespace crimson { } - bool remove_by_req_filter(std::function filter, - bool visit_backwards = false) { - struct Sink { - void push_back(const R& v) {} // do nothing - }; - static Sink my_sink; - return remove_by_req_filter(filter, my_sink, visit_backwards); - } - - - template - bool remove_by_req_filter(std::function filter, - Collect& out, + bool remove_by_req_filter(std::function filter_accum, bool visit_backwards = false) { bool any_removed = false; DataGuard g(data_mtx); for (auto i : client_map) { bool modified = - i.second->remove_by_req_filter(filter, out, visit_backwards); + i.second->remove_by_req_filter(filter_accum, visit_backwards); if (modified) { resv_heap.adjust(*i.second); limit_heap.adjust(*i.second); @@ -471,29 +453,33 @@ namespace crimson { } - void remove_by_client(const C& client) { - struct Sink { - void push_back(const R& v) {} - }; - static Sink my_sink; - remove_by_client(client, my_sink); + // use as a default value when no accumulator is provide + static void request_sink(const R& req) { + // do nothing } - // Collect must support calls to push_back(R), such as - // std::list. - template - void remove_by_client(const C& client, Collect& out) { + void remove_by_client(const C& client, + bool reverse = false, + std::function accum = request_sink) { DataGuard g(data_mtx); auto i = client_map.find(client); if (i == client_map.end()) return; - for (auto j = i->second->requests.begin(); - j != i->second->requests.end(); - ++j) { - out.push_back(*j->request); + if (reverse) { + for (auto j = i->second->requests.rbegin(); + j != i->second->requests.rend(); + ++j) { + accum(*j->request); + } + } else { + for (auto j = i->second->requests.begin(); + j != i->second->requests.end(); + ++j) { + accum(*j->request); + } } i->second->requests.clear(); diff --git a/test/test_dmclock_server.cc b/test/test_dmclock_server.cc index def45e3..ef3c5a1 100644 --- a/test/test_dmclock_server.cc +++ b/test/test_dmclock_server.cc @@ -263,8 +263,16 @@ namespace crimson { EXPECT_EQ(5, pq.request_count()); std::list capture; - pq.remove_by_req_filter([](const MyReq& r) -> bool {return 0 == r.id % 2;}, - capture); + pq.remove_by_req_filter( + [&capture] (const MyReq& r) -> bool { + if (0 == r.id % 2) { + capture.push_front(r); + return true; + } else { + return false; + } + }, + true); EXPECT_EQ(0, pq.request_count()); EXPECT_EQ(5, capture.size()); @@ -276,7 +284,89 @@ namespace crimson { } // TEST - TEST(dmclock_server, remove_by_req_filter_ordering) { + TEST(dmclock_server, remove_by_req_filter_ordering_forwards_visit) { + struct MyReq { + int id; + + MyReq(int _id) : + id(_id) + { + // empty + } + }; // MyReq + + using ClientId = int; + using Queue = dmc::PullPriorityQueue; + + ClientId client1 = 17; + + dmc::ClientInfo info1(0.0, 1.0, 0.0); + + auto client_info_f = [&] (ClientId c) -> dmc::ClientInfo { + return info1; + }; + + Queue pq(client_info_f, true); + + EXPECT_EQ(0, pq.client_count()); + EXPECT_EQ(0, pq.request_count()); + + ReqParams req_params(1,1); + + pq.add_request(MyReq(1), client1, req_params); + pq.add_request(MyReq(2), client1, req_params); + pq.add_request(MyReq(3), client1, req_params); + pq.add_request(MyReq(4), client1, req_params); + pq.add_request(MyReq(5), client1, req_params); + pq.add_request(MyReq(6), client1, req_params); + + EXPECT_EQ(1, pq.client_count()); + EXPECT_EQ(6, pq.request_count()); + + // remove odd ids in forward order and append to end + + std::vector capture; + pq.remove_by_req_filter( + [&capture] (const MyReq& r) -> bool { + if (1 == r.id % 2) { + capture.push_back(r); + return true; + } else { + return false; + } + }, + false); + + EXPECT_EQ(3, pq.request_count()); + EXPECT_EQ(3, capture.size()); + EXPECT_EQ(1, capture[0].id) << "items should come out in forward order"; + EXPECT_EQ(3, capture[1].id) << "items should come out in forward order"; + EXPECT_EQ(5, capture[2].id) << "items should come out in forward order"; + + // remove even ids in reverse order but insert at front so comes + // out forwards + + std::vector capture2; + pq.remove_by_req_filter( + [&capture2] (const MyReq& r) -> bool { + if (0 == r.id % 2) { + capture2.insert(capture2.begin(), r); + return true; + } else { + return false; + } + }, + false); + + EXPECT_EQ(0, pq.request_count()); + EXPECT_EQ(3, capture2.size()); + EXPECT_EQ(6, capture2[0].id) << "items should come out in reverse order"; + EXPECT_EQ(4, capture2[1].id) << "items should come out in reverse order"; + EXPECT_EQ(2, capture2[2].id) << "items should come out in reverse order"; + } // TEST + + + TEST(dmclock_server, remove_by_req_filter_ordering_backwards_visit) { struct MyReq { int id; @@ -318,8 +408,16 @@ namespace crimson { // now remove odd ids in forward order std::vector capture; - pq.remove_by_req_filter([](const MyReq& r) -> bool {return 1 == r.id % 2;}, - capture); + pq.remove_by_req_filter( + [&capture] (const MyReq& r) -> bool { + if (1 == r.id % 2) { + capture.insert(capture.begin(), r); + return true; + } else { + return false; + } + }, + true); EXPECT_EQ(3, pq.request_count()); EXPECT_EQ(3, capture.size()); @@ -330,9 +428,17 @@ namespace crimson { // now remove even ids in reverse order std::vector capture2; - pq.remove_by_req_filter([](const MyReq& r) -> bool {return 0 == r.id % 2;}, - capture2, - true); + pq.remove_by_req_filter( + [&capture2] (const MyReq& r) -> bool { + if (0 == r.id % 2) { + capture2.push_back(r); + return true; + } else { + return false; + } + }, + true); + EXPECT_EQ(0, pq.request_count()); EXPECT_EQ(3, capture2.size()); EXPECT_EQ(6, capture2[0].id) << "items should come out in reverse order"; @@ -386,7 +492,11 @@ namespace crimson { std::list removed; - pq.remove_by_client(client1, removed); + pq.remove_by_client(client1, + true, + [&removed] (const MyReq& r) { + removed.push_front(r); + }); EXPECT_EQ(3, removed.size()); EXPECT_EQ(1, removed.front().id);