Skip to content

Commit

Permalink
enforce move semantics of request types
Browse files Browse the repository at this point in the history 
this requires the request type support move semantics, but the upside is
that the request type will not be copied under any circumstances.

Signed-off-by: Kefu Chai <[email protected]>
  • Loading branch information
@tchaikov
tchaikov committed Jul 13, 2017
1 parent b84ccf9 commit f4b155d
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 49 deletions.
7 changes: 4 additions & 3 deletions sim/src/sim_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ namespace crimson {

using SubmitFunc =
std::function<void(const ServerId&,
const TestRequest&,
TestRequest&&,
const ClientId&,
const ReqPm&)>;

Expand Down Expand Up @@ -240,8 +240,9 @@ namespace crimson {
count_stats(internal_stats.mtx,
internal_stats.get_req_params_count);

TestRequest req(server, o, 12);
submit_f(server, req, id, rp);
submit_f(server,
TestRequest{server, static_cast<uint32_t>(o), 12},
id, rp);
++outstanding_ops;
l.lock(); // lock for return to top of loop

Expand Down
8 changes: 4 additions & 4 deletions sim/src/sim_server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,14 +147,15 @@ namespace crimson {
delete priority_queue;
}

void post(const TestRequest& request,
void post(TestRequest&& request,
const ClientId& client_id,
const ReqPm& req_params)
{
time_stats(internal_stats.mtx,
internal_stats.add_request_time,
[&](){
priority_queue->add_request(request, client_id, req_params);
priority_queue->add_request(std::move(request),
client_id, req_params);
});
count_stats(internal_stats.mtx,
internal_stats.add_request_count);
Expand Down Expand Up @@ -202,10 +203,9 @@ namespace crimson {
// notify server of completion
std::this_thread::sleep_for(op_time);

TestResponse resp(req->epoch);
// TODO: rather than assuming this constructor exists, perhaps
// pass in a function that does this mapping?
client_resp_f(client, resp, id, additional);
client_resp_f(client, TestResponse{req->epoch}, id, additional);

time_stats(internal_stats.mtx,
internal_stats.request_complete_time,
Expand Down
5 changes: 3 additions & 2 deletions sim/src/ssched/ssched_server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,11 @@ namespace crimson {
finishing = true;
}

void add_request(const R& request,
void add_request(R&& request,
const C& client_id,
const ReqParams& req_params) {
add_request(RequestRef(new R(request)), client_id, req_params);
add_request(RequestRef(new R(std::move(request))),
client_id, req_params);
}

void add_request(RequestRef&& request,
Expand Down
4 changes: 2 additions & 2 deletions sim/src/test_dmclock_main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,11 @@ int main(int argc, char* argv[]) {
// lambda to post a request to the identified server; called by client
test::SubmitFunc server_post_f =
[&simulation](const ServerId& server,
const sim::TestRequest& request,
sim::TestRequest&& request,
const ClientId& client_id,
const test::dmc::ReqParams& req_params) {
test::DmcServer& s = simulation->get_server(server);
s.post(request, client_id, req_params);
s.post(std::move(request), client_id, req_params);
};

std::vector<std::vector<sim::CliInst>> cli_inst;
Expand Down
4 changes: 2 additions & 2 deletions sim/src/test_ssched_main.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ int main(int argc, char* argv[]) {
// lambda to post a request to the identified server; called by client
test::SubmitFunc server_post_f =
[&simulation](const ServerId& server_id,
const sim::TestRequest& request,
sim::TestRequest&& request,
const ClientId& client_id,
const ssched::ReqParams& req_params) {
auto& server = simulation->get_server(server_id);
server.post(request, client_id, req_params);
server.post(std::move(request), client_id, req_params);
};

static std::vector<sim::CliInst> no_wait =
Expand Down
36 changes: 18 additions & 18 deletions src/dmclock_server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,12 +371,12 @@ namespace crimson {

// NB: because a deque is the underlying structure, this
// operation might be expensive
bool remove_by_req_filter_fw(std::function<bool(const R&)> filter_accum) {
bool remove_by_req_filter_fw(std::function<bool(R&&)> filter_accum) {
bool any_removed = false;
for (auto i = requests.begin();
i != requests.end();
/* no inc */) {
if (filter_accum(*i->request)) {
if (filter_accum(std::move(*i->request))) {
any_removed = true;
i = requests.erase(i);
} else {
Expand All @@ -388,12 +388,12 @@ namespace crimson {

// NB: because a deque is the underlying structure, this
// operation might be expensive
bool remove_by_req_filter_bw(std::function<bool(const R&)> filter_accum) {
bool remove_by_req_filter_bw(std::function<bool(R&&)> filter_accum) {
bool any_removed = false;
for (auto i = requests.rbegin();
i != requests.rend();
/* no inc */) {
if (filter_accum(*i->request)) {
if (filter_accum(std::move(*i->request))) {
any_removed = true;
i = decltype(i){ requests.erase(std::next(i).base()) };
} else {
Expand All @@ -404,7 +404,7 @@ namespace crimson {
}

inline bool
remove_by_req_filter(std::function<bool(const R&)> filter_accum,
remove_by_req_filter(std::function<bool(R&&)> filter_accum,
bool visit_backwards) {
if (visit_backwards) {
return remove_by_req_filter_bw(filter_accum);
Expand Down Expand Up @@ -478,7 +478,7 @@ namespace crimson {
}


bool remove_by_req_filter(std::function<bool(const R&)> filter_accum,
bool remove_by_req_filter(std::function<bool(R&&)> filter_accum,
bool visit_backwards = false) {
bool any_removed = false;
DataGuard g(data_mtx);
Expand All @@ -500,14 +500,14 @@ namespace crimson {


// use as a default value when no accumulator is provide
static void request_sink(const R& req) {
static void request_sink(R&& req) {
// do nothing
}


void remove_by_client(const C& client,
bool reverse = false,
std::function<void (const R&)> accum = request_sink) {
std::function<void (R&&)> accum = request_sink) {
DataGuard g(data_mtx);

auto i = client_map.find(client);
Expand All @@ -518,13 +518,13 @@ namespace crimson {
for (auto j = i->second->requests.rbegin();
j != i->second->requests.rend();
++j) {
accum(*j->request);
accum(std::move(*j->request));
}
} else {
for (auto j = i->second->requests.begin();
j != i->second->requests.end();
++j) {
accum(*j->request);
accum(std::move(*j->request));
}
}

Expand Down Expand Up @@ -1162,23 +1162,23 @@ namespace crimson {
}


inline void add_request(const R& request,
inline void add_request(R&& request,
const C& client_id,
const ReqParams& req_params,
double addl_cost = 0.0) {
add_request(typename super::RequestRef(new R(request)),
add_request(typename super::RequestRef(new R(std::move(request))),
client_id,
req_params,
get_time(),
addl_cost);
}


inline void add_request(const R& request,
inline void add_request(R&& request,
const C& client_id,
double addl_cost = 0.0) {
static const ReqParams null_req_params;
add_request(typename super::RequestRef(new R(request)),
add_request(typename super::RequestRef(new R(std::move(request))),
client_id,
null_req_params,
get_time(),
Expand All @@ -1187,12 +1187,12 @@ namespace crimson {



inline void add_request_time(const R& request,
inline void add_request_time(R&& request,
const C& client_id,
const ReqParams& req_params,
const Time time,
double addl_cost = 0.0) {
add_request(typename super::RequestRef(new R(request)),
add_request(typename super::RequestRef(new R(std::move(request))),
client_id,
req_params,
time,
Expand Down Expand Up @@ -1402,11 +1402,11 @@ namespace crimson {

public:

inline void add_request(const R& request,
inline void add_request(R&& request,
const C& client_id,
const ReqParams& req_params,
double addl_cost = 0.0) {
add_request(typename super::RequestRef(new R(request)),
add_request(typename super::RequestRef(new R(std::move(request))),
client_id,
req_params,
get_time(),
Expand Down
29 changes: 11 additions & 18 deletions test/test_dmclock_server.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,19 @@ namespace crimson {
};

QueueRef pq(new Queue(client_info_f, false));
Request req;
ReqParams req_params(1,1);

// Disable coredumps
PrCtl unset_dumpable;

EXPECT_DEATH_IF_SUPPORTED(pq->add_request(req, client1, req_params),
EXPECT_DEATH_IF_SUPPORTED(pq->add_request(Request{}, client1, req_params),
"Assertion.*reservation.*max_tag.*"
"proportion.*max_tag") <<
"we should fail if a client tries to generate a reservation tag "
"where reservation and proportion are both 0";


EXPECT_DEATH_IF_SUPPORTED(pq->add_request(req, client2, req_params),
EXPECT_DEATH_IF_SUPPORTED(pq->add_request(Request{}, client2, req_params),
"Assertion.*reservation.*max_tag.*"
"proportion.*max_tag") <<
"we should fail if a client tries to generate a reservation tag "
Expand Down Expand Up @@ -554,14 +553,13 @@ namespace crimson {

pq = QueueRef(new Queue(client_info_f, false));

Request req;
ReqParams req_params(1,1);

auto now = dmc::get_time();

for (int i = 0; i < 5; ++i) {
pq->add_request(req, client1, req_params);
pq->add_request(req, client2, req_params);
pq->add_request(Request{}, client1, req_params);
pq->add_request(Request{}, client2, req_params);
now += 0.0001;
}

Expand Down Expand Up @@ -608,15 +606,14 @@ namespace crimson {

QueueRef pq(new Queue(client_info_f, false));

Request req;
ReqParams req_params(1,1);

// make sure all times are well before now
auto old_time = dmc::get_time() - 100.0;

for (int i = 0; i < 5; ++i) {
pq->add_request_time(req, client1, req_params, old_time);
pq->add_request_time(req, client2, req_params, old_time);
pq->add_request_time(Request{}, client1, req_params, old_time);
pq->add_request_time(Request{}, client2, req_params, old_time);
old_time += 0.001;
}

Expand Down Expand Up @@ -667,16 +664,15 @@ namespace crimson {

QueueRef pq(new Queue(client_info_f, false));

Request req;
ReqParams req_params(1,1);

// make sure all times are well before now
auto start_time = dmc::get_time() - 100.0;

// add six requests; for same client reservations spaced one apart
for (int i = 0; i < 3; ++i) {
pq->add_request_time(req, client1, req_params, start_time);
pq->add_request_time(req, client2, req_params, start_time);
pq->add_request_time(Request{}, client1, req_params, start_time);
pq->add_request_time(Request{}, client2, req_params, start_time);
}

Queue::PullReq pr = pq->pull_request(start_time + 0.5);
Expand Down Expand Up @@ -750,13 +746,12 @@ namespace crimson {

QueueRef pq(new Queue(client_info_f, false));

Request req;
ReqParams req_params(1,1);

// make sure all times are well before now
auto now = dmc::get_time();

pq->add_request_time(req, client1, req_params, now + 100);
pq->add_request_time(Request{}, client1, req_params, now + 100);
Queue::PullReq pr = pq->pull_request(now);

EXPECT_EQ(Queue::NextReqType::future, pr.type);
Expand All @@ -782,13 +777,12 @@ namespace crimson {

QueueRef pq(new Queue(client_info_f, true));

Request req;
ReqParams req_params(1,1);

// make sure all times are well before now
auto now = dmc::get_time();

pq->add_request_time(req, client1, req_params, now + 100);
pq->add_request_time(Request{}, client1, req_params, now + 100);
Queue::PullReq pr = pq->pull_request(now);

EXPECT_EQ(Queue::NextReqType::returning, pr.type);
Expand All @@ -814,13 +808,12 @@ namespace crimson {

QueueRef pq(new Queue(client_info_f, true));

Request req;
ReqParams req_params(1,1);

// make sure all times are well before now
auto now = dmc::get_time();

pq->add_request_time(req, client1, req_params, now + 100);
pq->add_request_time(Request{}, client1, req_params, now + 100);
Queue::PullReq pr = pq->pull_request(now);

EXPECT_EQ(Queue::NextReqType::returning, pr.type);
Expand Down

0 comments on commit f4b155d

Please sign in to comment.