From d331aa2550f2a3588d3d114fa0595e823778bbf5 Mon Sep 17 00:00:00 2001 From: Michael Butler Date: Mon, 2 Sep 2019 16:37:33 -0700 Subject: Simplify VersionedInterfaces error handling logic This CL has the following simplifications: * CHECK input is non-null instead of gracefully failing * more clearly separate versioned logic * remove std::nothrow when calling "new" for small objects * makes const methods const Bug: N/A Test: mma Test: NeuralNetworksTest_static Test: CtsNNAPITestCases Change-Id: If739b4cc37d8fd0d6b844111327c0688057c27e1 Merged-In: If739b4cc37d8fd0d6b844111327c0688057c27e1 (cherry picked from commit c736a7cbd3b981d8104243d8c9ece4978cc05710) --- nn/runtime/VersionedInterfaces.cpp | 189 ++++++++++++++++++++----------------- nn/runtime/VersionedInterfaces.h | 40 ++++---- 2 files changed, 120 insertions(+), 109 deletions(-) diff --git a/nn/runtime/VersionedInterfaces.cpp b/nn/runtime/VersionedInterfaces.cpp index fd8532665..15da73e7f 100644 --- a/nn/runtime/VersionedInterfaces.cpp +++ b/nn/runtime/VersionedInterfaces.cpp @@ -156,17 +156,13 @@ class IPreparedModelDeathHandler : public DeathHandler {}; static std::shared_ptr makeVersionedIPreparedModel( sp preparedModel) { // verify input - if (!preparedModel) { - LOG(ERROR) << "makeVersionedIPreparedModel -- passed invalid preparedModel object."; + if (preparedModel == nullptr) { + LOG(ERROR) << "makeVersionedIPreparedModel passed invalid preparedModel object."; return nullptr; } // create death handler object - sp deathHandler = new (std::nothrow) IPreparedModelDeathHandler(); - if (!deathHandler) { - LOG(ERROR) << "makeVersionedIPreparedModel -- Failed to create IPreparedModelDeathHandler."; - return nullptr; - } + sp deathHandler = new IPreparedModelDeathHandler(); // linkToDeath registers a callback that will be invoked on service death to // proactively handle service crashes. If the linkToDeath call fails, @@ -174,7 +170,7 @@ static std::shared_ptr makeVersionedIPreparedModel( // providing the response. const Return ret = preparedModel->linkToDeath(deathHandler, 0); if (!ret.isOk() || ret != true) { - LOG(ERROR) << "makeVersionedIPreparedModel -- Failed to register a death recipient for the " + LOG(ERROR) << "makeVersionedIPreparedModel failed to register a death recipient for the " "IPreparedModel object."; return nullptr; } @@ -199,7 +195,7 @@ VersionedIPreparedModel::~VersionedIPreparedModel() { } std::tuple, Timing> VersionedIPreparedModel::executeAsynchronously( - const Request& request, MeasureTiming measure) { + const Request& request, MeasureTiming measure) const { const auto failWithStatus = [](ErrorStatus status) { return getExecutionResult(status, {}, kNoTiming); }; @@ -246,7 +242,7 @@ std::tuple, Timing> VersionedIPreparedModel::execu } std::tuple, Timing> VersionedIPreparedModel::executeSynchronously( - const Request& request, MeasureTiming measure) { + const Request& request, MeasureTiming measure) const { const auto kFailure = getExecutionResult(ErrorStatus::GENERAL_FAILURE, {}, kNoTiming); // version 1.2+ HAL @@ -270,7 +266,7 @@ std::tuple, Timing> VersionedIPreparedModel::execu } std::tuple, Timing> VersionedIPreparedModel::execute( - const Request& request, MeasureTiming measure, bool preferSynchronous) { + const Request& request, MeasureTiming measure, bool preferSynchronous) const { if (preferSynchronous) { VLOG(EXECUTION) << "Before executeSynchronously() " << SHOW_IF_DEBUG(toString(request)); return executeSynchronously(request, measure); @@ -282,18 +278,17 @@ std::tuple, Timing> VersionedIPreparedModel::execu std::shared_ptr VersionedIPreparedModel::configureExecutionBurst( bool blocking) const { - if (mPreparedModelV1_2 != nullptr) { - return ExecutionBurstController::create(mPreparedModelV1_2, blocking); - } else { + if (mPreparedModelV1_2 == nullptr) { return nullptr; } + return ExecutionBurstController::create(mPreparedModelV1_2, blocking); } std::shared_ptr VersionedIDevice::create(std::string serviceName, sp device) { auto core = Core::create(std::move(device)); if (!core.has_value()) { - LOG(ERROR) << "VersionedIDevice::create -- Failed to create Core."; + LOG(ERROR) << "VersionedIDevice::create failed to create Core."; return nullptr; } @@ -306,17 +301,10 @@ VersionedIDevice::VersionedIDevice(std::string serviceName, Core core) std::optional VersionedIDevice::Core::create(sp device) { // verify input - if (!device) { - LOG(ERROR) << "VersionedIDevice::Core::create -- passed invalid device object."; - return {}; - } + CHECK(device != nullptr) << "VersionedIDevice::Core::create passed invalid device object."; // create death handler object - sp deathHandler = new (std::nothrow) IDeviceDeathHandler(); - if (!deathHandler) { - LOG(ERROR) << "VersionedIDevice::Core::create -- Failed to create IDeviceDeathHandler."; - return {}; - } + sp deathHandler = new IDeviceDeathHandler(); // linkToDeath registers a callback that will be invoked on service death to // proactively handle service crashes. If the linkToDeath call fails, @@ -324,9 +312,8 @@ std::optional VersionedIDevice::Core::create(sp ret = device->linkToDeath(deathHandler, 0); if (!ret.isOk() || ret != true) { - LOG(ERROR) - << "VersionedIDevice::Core::create -- Failed to register a death recipient for the " - "IDevice object."; + LOG(ERROR) << "VersionedIDevice::Core::create failed to register a death recipient for the " + "IDevice object."; return {}; } @@ -449,7 +436,7 @@ Return VersionedIDevice::recoverable( auto core = Core::create(std::move(recoveredDevice)); if (!core.has_value()) { - LOG(ERROR) << "VersionedIDevice::recoverable -- Failed to create Core."; + LOG(ERROR) << "VersionedIDevice::recoverable failed to create Core."; return ret; } @@ -470,10 +457,11 @@ Return VersionedIDevice::recoverable( return ret; } -std::pair VersionedIDevice::getCapabilities() { +std::pair VersionedIDevice::getCapabilities() const { const std::pair kFailure = {ErrorStatus::GENERAL_FAILURE, {}}; std::pair result; + // version 1.2+ HAL if (getDevice() != nullptr) { NNTRACE_FULL(NNTRACE_LAYER_IPC, NNTRACE_PHASE_INITIALIZATION, "getCapabilities_1_2"); Return ret = recoverable( @@ -487,7 +475,11 @@ std::pair VersionedIDevice::getCapabilities() { LOG(ERROR) << "getCapabilities_1_2 failure: " << ret.description(); return {ErrorStatus::GENERAL_FAILURE, {}}; } - } else if (getDevice() != nullptr) { + return result; + } + + // version 1.1 HAL + if (getDevice() != nullptr) { NNTRACE_FULL(NNTRACE_LAYER_IPC, NNTRACE_PHASE_INITIALIZATION, "getCapabilities_1_1"); Return ret = recoverable( __FUNCTION__, [&result](const sp& device) { @@ -501,7 +493,11 @@ std::pair VersionedIDevice::getCapabilities() { LOG(ERROR) << "getCapabilities_1_1 failure: " << ret.description(); return kFailure; } - } else if (getDevice() != nullptr) { + return result; + } + + // version 1.0 HAL + if (getDevice() != nullptr) { NNTRACE_FULL(NNTRACE_LAYER_IPC, NNTRACE_PHASE_INITIALIZATION, "getCapabilities"); Return ret = recoverable( __FUNCTION__, [&result](const sp& device) { @@ -515,17 +511,19 @@ std::pair VersionedIDevice::getCapabilities() { LOG(ERROR) << "getCapabilities failure: " << ret.description(); return kFailure; } - } else { - LOG(ERROR) << "Device not available!"; - return {ErrorStatus::DEVICE_UNAVAILABLE, {}}; + return result; } - return result; + // No device available + LOG(ERROR) << "Device not available!"; + return {ErrorStatus::DEVICE_UNAVAILABLE, {}}; } -std::pair> VersionedIDevice::getSupportedExtensions() { +std::pair> VersionedIDevice::getSupportedExtensions() const { const std::pair> kFailure = {ErrorStatus::GENERAL_FAILURE, {}}; NNTRACE_FULL(NNTRACE_LAYER_IPC, NNTRACE_PHASE_COMPILATION, "getSupportedExtensions"); + + // version 1.2+ HAL if (getDevice() != nullptr) { std::pair> result; Return ret = recoverable( @@ -540,16 +538,20 @@ std::pair> VersionedIDevice::getSupportedExtens return kFailure; } return result; - } else if (getDevice() != nullptr) { + } + + // version too low + if (getDevice() != nullptr) { return {ErrorStatus::NONE, {/* No extensions. */}}; - } else { - LOG(ERROR) << "Device not available!"; - return {ErrorStatus::DEVICE_UNAVAILABLE, {}}; } + + // No device available + LOG(ERROR) << "Device not available!"; + return {ErrorStatus::DEVICE_UNAVAILABLE, {}}; } std::pair> VersionedIDevice::getSupportedOperations( - const MetaModel& metaModel) { + const MetaModel& metaModel) const { const std::pair> kFailure = {ErrorStatus::GENERAL_FAILURE, {}}; std::pair> result; @@ -576,6 +578,7 @@ std::pair> VersionedIDevice::getSupportedOperations( return std::make_pair(status, std::move(remappedSupported)); }; + // version 1.2+ HAL if (getDevice() != nullptr) { NNTRACE_FULL(NNTRACE_LAYER_IPC, NNTRACE_PHASE_COMPILATION, "getSupportedOperations_1_2"); Return ret = recoverable( @@ -592,6 +595,7 @@ std::pair> VersionedIDevice::getSupportedOperations( return result; } + // version 1.1 HAL if (getDevice() != nullptr) { const bool compliant = compliantWithV1_1(model); V1_1::Model model11; @@ -623,6 +627,7 @@ std::pair> VersionedIDevice::getSupportedOperations( return result; } + // version 1.0 HAL if (getDevice() != nullptr) { const bool compliant = compliantWithV1_0(model); V1_0::Model model10; @@ -654,20 +659,18 @@ std::pair> VersionedIDevice::getSupportedOperations( return result; } + // No device available + LOG(ERROR) << "Device not available!"; return kFailure; } std::pair> VersionedIDevice::prepareModel( const Model& model, ExecutionPreference preference, const hidl_vec& modelCache, - const hidl_vec& dataCache, const CacheToken& token) { + const hidl_vec& dataCache, const CacheToken& token) const { const std::pair> kFailure = { ErrorStatus::GENERAL_FAILURE, nullptr}; - const sp callback = new (std::nothrow) PreparedModelCallback(); - if (callback == nullptr) { - LOG(ERROR) << "prepareModel failed to create callback object"; - return kFailure; - } + const sp callback = new PreparedModelCallback(); // If 1.2 device, try preparing model if (getDevice() != nullptr) { @@ -778,17 +781,13 @@ std::pair> VersionedIDevic std::pair> VersionedIDevice::prepareModelFromCache(const hidl_vec& modelCache, const hidl_vec& dataCache, - const CacheToken& token) { + const CacheToken& token) const { const std::pair> kFailure = { ErrorStatus::GENERAL_FAILURE, nullptr}; - const sp callback = new (std::nothrow) PreparedModelCallback(); - if (callback == nullptr) { - LOG(ERROR) << "prepareModelFromCache failed to create callback object"; - return kFailure; - } - + // version 1.2+ HAL if (getDevice() != nullptr) { + const sp callback = new PreparedModelCallback(); const Return ret = recoverable( __FUNCTION__, [&modelCache, &dataCache, &token, &callback](const sp& device) { @@ -808,32 +807,36 @@ VersionedIDevice::prepareModelFromCache(const hidl_vec& modelCache, return {callback->getStatus(), makeVersionedIPreparedModel(callback->getPreparedModel())}; } - if (getDevice() != nullptr || getDevice() != nullptr) { + // version too low + if (getDevice() != nullptr) { LOG(ERROR) << "prepareModelFromCache called on V1_1 or V1_0 device"; return kFailure; } + // No device available LOG(ERROR) << "prepareModelFromCache called with no device"; return kFailure; } -DeviceStatus VersionedIDevice::getStatus() { - if (getDevice() == nullptr) { - LOG(ERROR) << "Device not available!"; - return DeviceStatus::UNKNOWN; - } - - Return ret = recoverable( - __FUNCTION__, [](const sp& device) { return device->getStatus(); }); +DeviceStatus VersionedIDevice::getStatus() const { + // version 1.0+ HAL + if (getDevice() != nullptr) { + Return ret = recoverable( + __FUNCTION__, [](const sp& device) { return device->getStatus(); }); - if (!ret.isOk()) { - LOG(ERROR) << "getStatus failure: " << ret.description(); - return DeviceStatus::UNKNOWN; + if (!ret.isOk()) { + LOG(ERROR) << "getStatus failure: " << ret.description(); + return DeviceStatus::UNKNOWN; + } + return static_cast(ret); } - return static_cast(ret); + + // No device available + LOG(ERROR) << "Device not available!"; + return DeviceStatus::UNKNOWN; } -int64_t VersionedIDevice::getFeatureLevel() { +int64_t VersionedIDevice::getFeatureLevel() const { constexpr int64_t kFailure = -1; if (getDevice() != nullptr) { @@ -850,31 +853,36 @@ int64_t VersionedIDevice::getFeatureLevel() { int32_t VersionedIDevice::getType() const { constexpr int32_t kFailure = -1; - std::pair result; + // version 1.2+ HAL if (getDevice() != nullptr) { + int32_t result = kFailure; Return ret = recoverable( __FUNCTION__, [&result](const sp& device) { return device->getType([&result](ErrorStatus error, DeviceType deviceType) { - result = std::make_pair(error, deviceType); + if (error == ErrorStatus::NONE) { + result = static_cast(deviceType); + } }); }); if (!ret.isOk()) { LOG(ERROR) << "getType failure: " << ret.description(); return kFailure; } - return static_cast(result.second); - } else { - LOG(INFO) << "Unknown NNAPI device type."; - return ANEURALNETWORKS_DEVICE_UNKNOWN; + return result; } + + // version too low or no device available + LOG(INFO) << "Unknown NNAPI device type."; + return ANEURALNETWORKS_DEVICE_UNKNOWN; } -std::pair VersionedIDevice::getVersionString() { +std::pair VersionedIDevice::getVersionString() const { const std::pair kFailure = {ErrorStatus::GENERAL_FAILURE, ""}; - std::pair result; + // version 1.2+ HAL if (getDevice() != nullptr) { + std::pair result; Return ret = recoverable( __FUNCTION__, [&result](const sp& device) { return device->getVersionString( @@ -887,20 +895,25 @@ std::pair VersionedIDevice::getVersionString() { return kFailure; } return result; - } else if (getDevice() != nullptr || getDevice() != nullptr) { + } + + // version too low + if (getDevice() != nullptr) { return {ErrorStatus::NONE, "UNKNOWN"}; - } else { - LOG(ERROR) << "Could not handle getVersionString"; - return kFailure; } + + // No device available + LOG(ERROR) << "Could not handle getVersionString"; + return kFailure; } -std::tuple VersionedIDevice::getNumberOfCacheFilesNeeded() { +std::tuple VersionedIDevice::getNumberOfCacheFilesNeeded() const { constexpr std::tuple kFailure = {ErrorStatus::GENERAL_FAILURE, 0, 0}; - std::tuple result; + // version 1.2+ HAL if (getDevice() != nullptr) { + std::tuple result; Return ret = recoverable( __FUNCTION__, [&result](const sp& device) { return device->getNumberOfCacheFilesNeeded([&result](ErrorStatus error, @@ -914,12 +927,16 @@ std::tuple VersionedIDevice::getNumberOfCacheFi return kFailure; } return result; - } else if (getDevice() != nullptr || getDevice() != nullptr) { + } + + // version too low + if (getDevice() != nullptr) { return {ErrorStatus::NONE, 0, 0}; - } else { - LOG(ERROR) << "Could not handle getNumberOfCacheFilesNeeded"; - return kFailure; } + + // No device available + LOG(ERROR) << "Could not handle getNumberOfCacheFilesNeeded"; + return kFailure; } } // namespace nn diff --git a/nn/runtime/VersionedInterfaces.h b/nn/runtime/VersionedInterfaces.h index 43b41f3cc..7bbc2fb46 100644 --- a/nn/runtime/VersionedInterfaces.h +++ b/nn/runtime/VersionedInterfaces.h @@ -17,9 +17,8 @@ #ifndef ANDROID_FRAMEWORKS_ML_NN_RUNTIME_VERSIONED_INTERFACES_H #define ANDROID_FRAMEWORKS_ML_NN_RUNTIME_VERSIONED_INTERFACES_H -#include "HalInterfaces.h" - #include + #include #include #include @@ -28,7 +27,10 @@ #include #include #include +#include + #include "Callbacks.h" +#include "HalInterfaces.h" namespace android { namespace nn { @@ -98,7 +100,7 @@ class VersionedIDevice { * - GENERAL_FAILURE if there is an unspecified error * @return capabilities Capabilities of the driver. */ - std::pair getCapabilities(); + std::pair getCapabilities() const; /** * Gets information about extensions supported by the driver implementation. @@ -115,7 +117,7 @@ class VersionedIDevice { * - GENERAL_FAILURE if there is an unspecified error * @return extensions A list of supported extensions. */ - std::pair> getSupportedExtensions(); + std::pair> getSupportedExtensions() const; /** * Gets the supported operations in a MetaModel. @@ -146,7 +148,7 @@ class VersionedIDevice { * it is describing. */ std::pair> getSupportedOperations( - const MetaModel& metaModel); + const MetaModel& metaModel) const; /** * Synchronously creates a prepared model for execution and optionally saves it @@ -241,7 +243,7 @@ class VersionedIDevice { std::pair> prepareModel( const hal::Model& model, hal::ExecutionPreference preference, const hal::hidl_vec& modelCache, - const hal::hidl_vec& dataCache, const hal::CacheToken& token); + const hal::hidl_vec& dataCache, const hal::CacheToken& token) const; /** * Creates a prepared model from cache files for execution. @@ -311,7 +313,7 @@ class VersionedIDevice { */ std::pair> prepareModelFromCache( const hal::hidl_vec& modelCache, - const hal::hidl_vec& dataCache, const hal::CacheToken& token); + const hal::hidl_vec& dataCache, const hal::CacheToken& token) const; /** * Returns the current status of a driver. @@ -322,7 +324,7 @@ class VersionedIDevice { * - DeviceStatus::OFFLINE * - DeviceStatus::UNKNOWN */ - hal::DeviceStatus getStatus(); + hal::DeviceStatus getStatus() const; /** * Returns the feature level of a driver. @@ -333,7 +335,7 @@ class VersionedIDevice { * Return -1 if the driver is offline or busy, or the query resulted in * an unspecified error. */ - int64_t getFeatureLevel(); + int64_t getFeatureLevel() const; /** * Returns the device type of a driver. @@ -377,7 +379,7 @@ class VersionedIDevice { * @return version The version string of the device implementation. * Must have nonzero length if the query is successful, and must be an empty string if not. */ - std::pair getVersionString(); + std::pair getVersionString() const; /** * Gets the caching requirements of the driver implementation. @@ -417,14 +419,7 @@ class VersionedIDevice { * the driver needs to cache a single prepared model. It must * be less than or equal to Constant::MAX_NUMBER_OF_CACHE_FILES. */ - std::tuple getNumberOfCacheFilesNeeded(); - - /** - * Returns the name of the service that implements the driver - * - * @return serviceName The name of the service. - */ - std::string getServiceName() const { return mServiceName; } + std::tuple getNumberOfCacheFilesNeeded() const; private: /** @@ -681,9 +676,8 @@ class VersionedIPreparedModel { * UINT64_MAX. A driver may choose to report any time as UINT64_MAX, * indicating that measurement is not available. */ - std::tuple, hal::Timing> execute(const hal::Request& request, - hal::MeasureTiming measure, - bool preferSynchronous); + std::tuple, hal::Timing> execute( + const hal::Request& request, hal::MeasureTiming measure, bool preferSynchronous) const; /** * Creates a burst controller on a prepared model. @@ -697,9 +691,9 @@ class VersionedIPreparedModel { private: std::tuple, hal::Timing> executeAsynchronously( - const hal::Request& request, hal::MeasureTiming timing); + const hal::Request& request, hal::MeasureTiming timing) const; std::tuple, hal::Timing> executeSynchronously( - const hal::Request& request, hal::MeasureTiming measure); + const hal::Request& request, hal::MeasureTiming measure) const; /** * All versions of IPreparedModel are necessary because the preparedModel could be v1.0, -- cgit v1.2.3