Make HostManager own Devices #2393
Conversation
|
Also it's necessary to block until requests are done because otherwise this would be a use-after-free glow/lib/Runtime/HostManager/HostManager.cpp Line 154 in d8cbb67 |
| @@ -214,6 +215,12 @@ ThreadPoolExecutor::~ThreadPoolExecutor() { | |||
| void ThreadPoolExecutor::run(const DAGNode *root, | |||
| std::unique_ptr<Context> context, | |||
| RunIdentifierTy runId, ResultCBTy cb) { | |||
| // Don't process new requests if the executor is shutting down. | |||
| if (shuttingDown_) { | |||
| cb(runId, ResultCode::Canceled, std::move(context)); | |||
There was a problem hiding this comment.
nit, should it be a ResultCode::Failed? It never really started.
There was a problem hiding this comment.
@gcatron I don't know what these mean, it would be good if they had some comments saying what each of them is. Canceled sounds more correct though just from guessing because there was nothing wrong with the request, it just got there too late.
There was a problem hiding this comment.
If you tell me what each of these are, I can add a comment in this PR to the ResultCode enum
There was a problem hiding this comment.
Yeah, they are a bit vague, canceled sounds good.
| // requests from being serviced. | ||
| executor_->shutdown(); | ||
| assert(activeRequestCount_ == 0 && | ||
| "All requests should be finished when shutting downt HostManager."); |
jackm321
force-pushed
the
block_hostmanager_shutdown
branch
from
caabd5b
to
deff232
Compare
|
@gcatron thanks for the speedy review! |
tests/unittests/ExecutorTest.cpp
Outdated
| deviceManagerMap_.insert(std::make_pair(testDeviceId, deviceManager)); | ||
| testDeviceManagerMap_.insert(std::make_pair(testDeviceId, deviceManager)); | ||
| llvm::make_unique<TestDeviceManager>(deviceManagerThreads); | ||
| deviceManagerMap_.insert( |
There was a problem hiding this comment.
nit: deviceManagerMap_.emplace(testDeviceId, std::move(deviceManager)) and in other places.
There was a problem hiding this comment.
@rdzhabarov I didn't write that but yeah I agree it's better I'll change it
Description:
Make it so HostManager owns DeviceManagers and the rest of the runtime just uses them. Make the blocking on shutdown in Executor explicit to ensure that no inflight requests will be using DeviceManagers. Prevent processing new requests while shutting down.
Testing:
ninja allcd to downloaded_models and
../bin/resnet-runtime ../../tests/images/imagenetDocumentation:
Fixes #2384
Fixes #2297