search

browsermt / mts

Marian Translation Service
Apache License 2.0
18 stars 4 forks source link

Service should fully own workers and queue. #7

Closed kpu closed 3 years ago

kpu commented 3 years ago

The workers and the queue are properly owned by Service.

Currently they are shared pointers.

https://github.com/browsermt/mts/blob/41978b4c225138432883a8c1abc98e7a86a31c2c/src/bergamot/service.h#L27 https://github.com/browsermt/mts/blob/41978b4c225138432883a8c1abc98e7a86a31c2c/src/bergamot/service.h#L28

They should be member variables of Service. Remove the shared pointer. If Service launches threads it's also responsible for cleaning them up in its destruction. And order the member variables such that the threads are stopped before the queue is deallocated.

kpu commented 3 years ago

You'll want to put pcqueue before workers because workers depend on queue being alive.

jerinphilip commented 3 years ago

BatchTranslator (workers_) contains std::thread, which is only movable. Moving and rebuilding the graphs etc seems to be wasted computation. Is it okay keeping these as unique_ptrs?j

kpu commented 3 years ago

Why can't you just reserve and emplace_back the std::vector<BatchTranslator>?

jerinphilip commented 3 years ago

My bad, kept trying to use move semantics, took looking at cppreference for emplace_back to figure I could pass args directly to be forwarded. Done in commit referenced.

https://github.com/browsermt/mts/blob/e79286cbc590847c03a285a4a001fb99d444c514/src/bergamot/service.h#L30-L34