diff --git a/docs/Coding_Conventions_and_Standards.md b/docs/Coding_Conventions_and_Standards.md index 25dcff6f62..e1e0ec08bd 100644 --- a/docs/Coding_Conventions_and_Standards.md +++ b/docs/Coding_Conventions_and_Standards.md @@ -12,29 +12,93 @@ Google style from with a few * Allowed * Use a non-const reference for arguments that are modifiable but cannot be `nullptr` so the API clearly advertises the intent * Const correctness and usage of smart pointers (`shared_ptr` and `unique_ptr`) is expected. A non-const reference equates to "this is a non-null object that you can change but are not being given ownership of". +* Prefer passing `gsl::span` by value (or `std::span` when supported) as input arguments when passing const references to containers with contiguous storage (like `std::vector`). This allows the function to be container independent, and the argument to represent arbitrary memory spans or sub-spans. The below examples allow the client code to use either `std::vector` or `InlinedVector`. An instance of a `gsl::span` would be created automatically. +```cpp +/// Instead of +void foo(const std::vector&); + +/// Use to pass any contiguous const container containing int64_t +// Now you can seamlessly pass either `std::vector`, `InlinedVector`, `std::array` or `gsl::span` as an argument. +void foo(gsl::span); + +// Example with pointer to const data. Instead of +void foo(const std::vector&); + +// Use +void foo(gsl::span); +``` +* Prefer returning `gsl::span` by value instead of a const reference to a contiguous member container. Prefer returning `gsl::span` instead of a pointer referring to a chunk of memory. The size is also included in the span. +For example, +```cpp +// Instead of +const std::vector& foo(); + +// Return a span by value +gsl::span foo(); + +// Instead of +const int64_t* foo(); + +// Return a span by value +gsl::span foo(); + +``` +* However, `std::initializer_list` is not automatically convertible to a `gsl::span`. Use `AsSpan({1, 2, 3})` defined at `core/common/span_utils.h` to convert `std::initializer_list` to a span. You can also use `std::array`. For example, +```cpp +// Original code +void foo(const std::vector&); + +foo({"abc", "dbf"}); // Works + +// After refactoring to gsl::span it would no longer compile. Use AsSpan(). +void foo(gsl::span); + +foo(AsSpan{"abc", "dbf"}); // Works +``` +* Prefer passing `std::string_view` by value instead of `const std::string&`. Make sure that the lifespan of a `std::string` instance ecplises the lifespan of the corresponding `std::string_view` instance. + * `using namespace` permitted with limited scope * Not allowing `using namespace` at all is overly restrictive. Follow the C++ Core Guidelines: * [SF.6: Use using namespace directives for transition, for foundation libraries (such as std), or within a local scope (only)](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-using) * [SF.7: Don't write using namespace at global scope in a header file](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-using-directive) -Other +### Containers to use + +Onnxruntime aims to reduce latency and latency variance by minimizing the amount of dynamic memory allocations. + +* The use of the following container `typedef`s to reduce memory allocations is required: + * Use `TensorShapeVector` typedef to build or modify shapes from `core/framework/tensor_shape.h`. It is based on a vector implementation that features small buffer optimization. Its small buffer size is the same to that of in TensorShape. + * Use `InlinedVector` typedef instead of std::vector defined at `core/common/inlined_containers_fwd.h`. By default, it provides 64 bytes of inlined storage. You can customize inlined size with the second template non-type parameter N. + * Use `InlinedHashSet` and `InlinedHashMap` typedefs from `core/common/inlined_containers.h`. These are drop-in replacements for `std::unordered_set/map` that store their keys and values in one continuous buffer and reduce the number of allocations. They also do not allocate an `end` node when default constructed. Note, that these Hash containers do not provide pointer stability. `std::map` and `std::set` can often be replaced by hash containers as well. + * For the node based containers where pointer stability is required, use `NodeHashSet` and `NodeHashMap`. Although node based, they are more cache friendly. + * Use `core/common/inlined_containers_fwd.h` to forward declare any of the above container types. + * Consider using `std::string_view` for use in containers to reduce the number of allocations and avoid string duplication. Keep in mind that the lifespan of the objects being referred to must eclipse the lifespan of the corresponding `std::string_view`. + * We have selected to use `Abseil` library for the above typedefs. Abseil container documentation is [here](https://abseil.io/docs/cpp/guides/container#abseil-containers). + * Do not use `Abseil` library or `absl` namespace directly. We should be able to build Onnxruntime without Abseil. + * Use `onnxruntime/tools/natvis/abseil-cpp.natvis` for the above containers visualizations and debugging help in `VS Studio` and `VS Code`. +* Prefer using `reserve()` and not `resize()` on vectors. `resize()` default constructs all the elements for the size which can be expensive/noticeable even if the type is trivial. Default values are rarely used in practice and it becomes a waste. Construction like `std::vector(10, 0)` is the same as `resize()` and is potentially wasteful. +* Use `reserve()` on hash containers and vectors. For example, +```cpp +#include "core/common/inlined_containers.h" + +void foo(gsl::span names) { + // For local processing, names are still valid + // use std::string_view to avoid duplicate memory allocations. + // same code would work with std::unordered_set if built without Abseil + InlinedHashSet unique_names; + unique_names.reserve(names.size()); + unique_names.insert(names.cbegin(), names.cend()); +} +``` + + +### Other * Qualify usages of `auto` with `const`, `*`, `&` and `&&` where applicable to more clearly express the intent * When adding a new class, disable copy/assignment/move until you have a proven need for these capabilities. If a need arises, enable copy/assignment/move selectively, and when doing so validate that the implementation of the class supports what is being enabled. * Use `ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE` initially * See the other `ORT_DISALLOW_*` macros in -* Prefer passing `gsl::span` by value (or `std::span` when supported) as input arguments when passing const references to containers with contiguous storage (like `std::vector`). This allows to make the function container independent, represent arbitrary memory spans or pass sub-spans as an argument. -* Use `AsSpan({1, 2, 3})` to convert `std::initializer_list` to a span. You can also use `std::array`. -* Prefer passing `std::string_view` by value instead of `const std::string&`. -* Prefer returning `gsl::span` or `gsl::span` by value instead of a const reference or reference to a contiguous member container. -* The use of the following container `typedef`s to reduce memory allocations is preferred: - * Use `TensorShapeVector` typedef to build or modify shapes from `core/framework/tensor_shape.h`. It is based on a vector implementation that features small buffer optimization. Its small buffer size is the same to that of in TensorShape. Use `InlinedShapeVector` for shape related operations, but of different type. - * Use `InlinedVector` typedef instead of std::vector. By default, it provides 64 bytes of inlined storage. You can customize inlined size with the second template non-type parameter N. - * Use `InlinedHashSet` and `InlinedHashMap` typedefs from `core/framework/inlined_containers.h`. These are drop-in replacements for `std::unordered_set/map` that store their keys and values in one continuous buffer and reduce the number of allocations. They also do not allocate an `end` node. Note, that these Hash containers do not provide pointer stability. - * Consider using `std::string_view` to use in maps and sets to reduce the number of allocations and avoid string duplication. Keep in mind that the strings referred to must be alive. - * We have selected to use `Abseil` library for the above typedefs. Abseil container documentation is [here](https://abseil.io/docs/cpp/guides/container#abseil-containers). -* Prefer using `reserve()` and not `resize()` on vectors. `resize()` default constructs all the elements for the size which can be expensive/noticeable even if the type is trivial. Default values are rarely used in practice and it becomes a waste. Construction like `std::vector(10, 0)` is the same as `resize()` and is potentially wasteful. -* Use `reserve()` on hash containers or pass the number of items in the constructor. +* Sometimes, `std::unique_ptr` might be considered for delayed or optional construction of objects or members of classes. Instead, use `std::optional` as appropriate to reduce the number of allocations. * Don't use `else` after `return`. see: [https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return](https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return) * Don't overuse `std::shared_ptr`. Use `std::shared_ptr` only if it's not clear when and where the object will be de-allocated. See also: [https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-shared_ptr](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-shared_ptr) * Avoid using the `long` type, which could be either 32 bits or 64 bits.