From 097bab8d1e1230e35b85bb169618390c3a94ca98 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Thu, 6 May 2021 19:29:34 +1000 Subject: [PATCH] Cleanup a change to ExecutionFrame a little (#7576) * Reduce the binary size growth from this change. Minimal build grew by 7KB from this checkin. Firstly simplify the checking logic a little. Same checks are still done - just without using an extra layer of helpers. The issue being addressed by the original change only applies if you have a graph output where the shape wasn't able to be inferred. e.g. Reshape node with dynamic input causes downstream shapes to be unknown. If that is not the case, MergeShapeInfo in graph.cc would have resolved any differences between a specified output shape and the inferred output shape during Graph::Resolve. The issue does not apply to the execution frame used by the optimizer as the only time it would create a graph output is if it could constant fold all the way through, so MergeShapeInfo would have handled any difference in that case as well. Due to these considerations, wiring a logger in at the IExecutionFrame level isn't necessary if VerifyOutputSizes optionally overridden by an implementation that cares. * Address PR comments --- onnxruntime/core/framework/execution_frame.cc | 87 +++++++------------ onnxruntime/core/framework/execution_frame.h | 18 ++-- .../optimizer/optimizer_execution_frame.cc | 10 +-- .../optimizer/optimizer_execution_frame.h | 2 - 4 files changed, 44 insertions(+), 73 deletions(-) diff --git a/onnxruntime/core/framework/execution_frame.cc b/onnxruntime/core/framework/execution_frame.cc index 6386ab6407..fd91e09ac2 100644 --- a/onnxruntime/core/framework/execution_frame.cc +++ b/onnxruntime/core/framework/execution_frame.cc @@ -52,7 +52,6 @@ Status IExecutionFrame::SetOutputMLValue(int index, const OrtValue& ort_value) { } void IExecutionFrame::UpdateFeeds(const std::vector& feed_mlvalue_idxs, const std::vector& feeds) { - ORT_ENFORCE(feed_mlvalue_idxs.size() == feeds.size()); for (size_t idx = 0, end = feed_mlvalue_idxs.size(); idx < end; ++idx) { @@ -66,10 +65,8 @@ void IExecutionFrame::UpdateFeeds(const std::vector& feed_mlvalue_idxs, con } void IExecutionFrame::UpdateFetches(const std::vector& fetch_mlvalue_idxs, const std::vector& fetches, const std::unordered_map& initializers) { - ORT_ENFORCE(fetch_mlvalue_idxs.size() == fetches.size()); - if (!fetches.empty()) { fetch_mlvalue_idxs_ = fetch_mlvalue_idxs; @@ -138,7 +135,8 @@ OrtValue* IExecutionFrame::GetMutableNodeInputOrOutputMLValue(int index) { // This method is not thread safe! // Return S_OK and nullptr if index map to an value that is an unused optional input/output -Status IExecutionFrame::GetOrCreateNodeOutputMLValue(const int output_index, int output_arg_index, const TensorShape* shape, OrtValue*& p_ort_value, +Status IExecutionFrame::GetOrCreateNodeOutputMLValue(const int output_index, int output_arg_index, + const TensorShape* shape, OrtValue*& p_ort_value, const Node& node, size_t nnz) { auto status = Status::OK(); int ort_value_idx = GetNodeIdxToMLValueIdx(output_arg_index); @@ -158,8 +156,9 @@ Status IExecutionFrame::GetOrCreateNodeOutputMLValue(const int output_index, int " Requested shape:", shape ? shape->ToString() : "null"); } } else { - if (this->IsOutput(ort_value_idx)) { - VerifyOutputSizes(output_index, node, shape); + // shape is nullptr for traditional ML output values + if (shape != nullptr && IsOutput(ort_value_idx)) { + VerifyOutputSizes(output_index, node, *shape); } status = CreateNodeOutputMLValueImpl(*p_ort_value, ort_value_idx, shape, nnz); } @@ -168,51 +167,6 @@ Status IExecutionFrame::GetOrCreateNodeOutputMLValue(const int output_index, int return status; } -void IExecutionFrame::VerifyOutputSizes(int output_index, const Node& node, - const TensorShape* output_shape) { - ProtoHelperNodeContext protoContext(node); - OpNodeProtoHelper info(&protoContext); - - const onnx::TypeProto* outputproto = info.GetOutputType(output_index); - - if (outputproto == nullptr) { - return; - } - - if (outputproto->value_case() != onnx::TypeProto::kTensorType) { - return; - } - const auto& tensortype = outputproto->tensor_type(); - if (tensortype.has_shape()) { - const auto& shape = tensortype.shape(); - - int actual_num_dims = static_cast(output_shape->NumDimensions()); - int expected_num_dims = shape.dim_size(); - if (actual_num_dims != expected_num_dims) { - if (GetLogger() != nullptr) { - LOGS(*GetLogger(), WARNING) << "Number of dimension(" << actual_num_dims << ") of output shape didn't match " - << "with model's expected number("<< expected_num_dims <<") of dimension of output shape"; - } - return; - } - - for (uint32_t output_dim = 0, end = actual_num_dims; output_dim < end; ++output_dim) { - const auto dim = shape.dim(output_dim); - - if (dim.has_dim_value()) { - int64_t expected_size = dim.dim_value(); - int64_t actual_size = (*output_shape)[output_dim]; - if (expected_size != actual_size) { - if (GetLogger() != NULL) { - LOGS(*GetLogger(), WARNING) << "Actual dimension(" << actual_size << ") didn't match with expected dimension (" - << expected_size << ") for outputProtoIndex: " << output_index; - } - return; - } - } - } - } -} bool IExecutionFrame::TryGetInferredShape(int /*index*/, TensorShape& /*shape*/) const { // By default, there is not information about inferred shape, so this default // implementation always returns false. The derived class of IExecutionFrame @@ -735,10 +689,6 @@ Status ExecutionFrame::AllocateAsPerAllocationPlan(OrtValue& ort_value, int ort_ } } -const logging::Logger* ExecutionFrame::GetLogger() { - return &(session_state_.Logger()); -} - AllocatorPtr ExecutionFrame::GetAllocatorImpl(const OrtMemoryInfo& info) const { return session_state_.GetAllocator(info); } @@ -750,6 +700,33 @@ Status ExecutionFrame::CreateNodeOutputMLValueImpl(OrtValue& ort_value, int ort_ return AllocateAsPerAllocationPlan(ort_value, ort_value_idx, shape, nnz); } +void ExecutionFrame::VerifyOutputSizes(int output_index, const Node& node, const TensorShape& output_shape) { + const NodeArg* output_def = node.OutputDefs()[output_index]; + const auto* expected_shape = output_def->Shape(); + if (expected_shape == nullptr) { + // model didn't specify shape and shape inferencing wasn't able to calculate it so nothing to compare against + return; + } + + const size_t expected_rank = expected_shape->dim_size(); + bool compatible = expected_rank == output_shape.NumDimensions(); + if (compatible) { + for (size_t i = 0; i < expected_rank; ++i) { + const auto& expected_dim = expected_shape->dim().Get(static_cast(i)); + if (expected_dim.has_dim_value() && expected_dim.dim_value() != output_shape[i]) { + compatible = false; + break; + } + } + } + + if (!compatible) { + LOGS(session_state_.Logger(), WARNING) << "Expected shape from model of " << *expected_shape + << " does not match actual shape of " << output_shape + << " for output " << output_def->Name(); + } +} + Status ExecutionFrame::ReleaseMLValueImpl(int ort_value_idx) { ORT_RETURN_IF_ERROR(IExecutionFrame::ReleaseMLValueImpl(ort_value_idx)); TraceFree(ort_value_idx); diff --git a/onnxruntime/core/framework/execution_frame.h b/onnxruntime/core/framework/execution_frame.h index 66b30d8d06..977056e472 100644 --- a/onnxruntime/core/framework/execution_frame.h +++ b/onnxruntime/core/framework/execution_frame.h @@ -53,7 +53,8 @@ class IExecutionFrame { // Override the index-th output with ort_value Status SetOutputMLValue(int index, const OrtValue& ort_value); void UpdateFeeds(const std::vector& feed_mlvalue_idxs, const std::vector& feeds); - void UpdateFetches(const std::vector& fetch_mlvalue_idxs, const std::vector& fetches, const std::unordered_map& initializers); + void UpdateFetches(const std::vector& fetch_mlvalue_idxs, const std::vector& fetches, + const std::unordered_map& initializers); Status GetOutputs(const std::vector& fetch_mlvalue_idxs, std::vector& fetches); #endif @@ -61,11 +62,11 @@ class IExecutionFrame { // This method is not thread safe! // Return S_OK and nullptr if index map to an value that is an unused optional input/output // Shape is required for tensors but not traditional ML values. - Status GetOrCreateNodeOutputMLValue(const int index, int output_arg_index, const TensorShape* shape, + Status GetOrCreateNodeOutputMLValue(const int index, int output_arg_index, const TensorShape* shape, OrtValue*& p_ort_value, const Node& node, size_t nnz = 0); // This function try retrieve the inferred shapes for the given NodeArg index. - // If the retrival is sucessful, this function returns true and false otherwise. + // If the retrieval is successful, this function returns true and false otherwise. virtual bool TryGetInferredShape(int index, TensorShape& shape) const; /** @@ -97,10 +98,9 @@ class IExecutionFrame { return all_values_[ort_value_index]; } - void VerifyOutputSizes(int output_index, const onnxruntime::Node& node, - const onnxruntime::TensorShape* output_shape); - - virtual const logging::Logger* GetLogger() = 0; + // optional function that can check if the requested output_shape matched what was specified/inferred + // for the node. + virtual void VerifyOutputSizes(int /*output_index*/, const Node& /*node*/, const TensorShape& /*output_shape*/) {} virtual AllocatorPtr GetAllocatorImpl(const OrtMemoryInfo& info) const = 0; @@ -180,10 +180,10 @@ class ExecutionFrame final : public IExecutionFrame { private: ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE(ExecutionFrame); - const logging::Logger* GetLogger() override; AllocatorPtr GetAllocatorImpl(const OrtMemoryInfo& info) const override; Status ReleaseMLValueImpl(int ort_value_idx) override; Status CreateNodeOutputMLValueImpl(OrtValue& ort_value, int ort_value_idx, const TensorShape* shape, size_t nnz) override; + void VerifyOutputSizes(int output_index, const Node& node, const TensorShape& output_shape) override; Status CopyTensor(const Tensor& src, Tensor& dest) const override; common::Status AllocateAsPerAllocationPlan(OrtValue& ort_value, int ort_value_index, const TensorShape* shape, @@ -223,7 +223,7 @@ class ExecutionFrame final : public IExecutionFrame { // Given the input shapes of the executed graph, ExecutionFrame tries inferring // all symbolic shapes. inferred_shapes_[i] is the shape of OrtValue indexed // by i, if the key i exists. - // inferred_shapes_ is generated togehter with mem_patterns_. + // inferred_shapes_ is generated together with mem_patterns_. std::unordered_map inferred_shapes_; // Size of virtual memory allocated before any kernel execution. diff --git a/onnxruntime/core/optimizer/optimizer_execution_frame.cc b/onnxruntime/core/optimizer/optimizer_execution_frame.cc index 01b168118a..dc4d6c5e89 100644 --- a/onnxruntime/core/optimizer/optimizer_execution_frame.cc +++ b/onnxruntime/core/optimizer/optimizer_execution_frame.cc @@ -112,7 +112,7 @@ std::unique_ptr OptimizerExecutionFrame::Info::CreateKernel(cons // For optimizer, probably no need to pass feed_mlvalue_idxs, feeds to initialize IExecutionFrame. // If needed, the parameters of OptimizerExecutionFrame ctor can be changed later. -OptimizerExecutionFrame::OptimizerExecutionFrame(const Info& info, +OptimizerExecutionFrame::OptimizerExecutionFrame(const Info& info, const std::vector& fetch_mlvalue_idxs, const std::vector& fetches) : IExecutionFrame(info.GetMLValueNameIdxMap(), info.GetNodeIndexInfo(), fetch_mlvalue_idxs), @@ -120,10 +120,6 @@ OptimizerExecutionFrame::OptimizerExecutionFrame(const Info& info, Init(std::vector(), std::vector(), info.GetInitializers(), fetches); } -const logging::Logger* OptimizerExecutionFrame::GetLogger() { - return nullptr; -} - AllocatorPtr OptimizerExecutionFrame::GetAllocatorImpl(const OrtMemoryInfo& info) const { return info_.GetAllocator(info); } @@ -168,8 +164,8 @@ Status OptimizerExecutionFrame::CreateNodeOutputMLValueImpl(OrtValue& ort_value, auto element_type = static_cast(ml_type)->GetElementType(); AllocatorPtr allocator_ptr = info_.GetAllocator(); std::unique_ptr p_tensor = std::make_unique(element_type, - *shape, - allocator_ptr); + *shape, + allocator_ptr); auto ml_tensor = DataTypeImpl::GetType(); ort_value.Init(p_tensor.release(), ml_tensor, ml_tensor->GetDeleteFunc()); diff --git a/onnxruntime/core/optimizer/optimizer_execution_frame.h b/onnxruntime/core/optimizer/optimizer_execution_frame.h index 839b2c60d5..8292786219 100644 --- a/onnxruntime/core/optimizer/optimizer_execution_frame.h +++ b/onnxruntime/core/optimizer/optimizer_execution_frame.h @@ -88,8 +88,6 @@ class OptimizerExecutionFrame final : public IExecutionFrame { private: ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE(OptimizerExecutionFrame); - const logging::Logger* GetLogger() override; - AllocatorPtr GetAllocatorImpl(const OrtMemoryInfo& info) const override; Status CreateNodeOutputMLValueImpl(OrtValue& ort_value, int ort_value_idx, const TensorShape* shape, size_t nnz) override;