[Runtime] Make ADTObject POD container type

[wweic]

https://discuss.tvm.ai/t/discuss-runtime-array-containers-array-adt-string/4582

TODO

• Fix allocator.
• Destructor
• ADTObj container interface improve.
• Object lifecycle correctness.

[tqchen]

NOTE: You will likely need to change memory.h::Allocator interface to allow

Allocator::make_with_extra_space<ADTObj>(alignof(ObjectRef), size * sizeof(ObjectRef))

[wweic]

NOTE: You will likely need to change memory.h::Allocator interface to allow

Allocator::make_with_extra_space<ADTObj>(alignof(ObjectRef), size * sizeof(ObjectRef))

Thanks for coming up with the name. :-)

[wweic]

cc @tqchen @zhiics @icemelon9 @junrushao1994

[tqchen]

We care about alignment(otherwise it will leads to mysterious segfaults).

Please use std::aligned_storage, allocate an array of if. Make sure you at least get alignof(T), and the requested alignment is dividable by alignof(T)

As an alternative design, we can also do(so you don't need to pass the alignment argument)

Allocator.make_with_extra_elems<ADTObject, ObjectRef>(num_elem, args);

Which also allows you to get std::align_of the second argument, and use https://en.cppreference.com/w/cpp/types/aligned_storage

[tqchen]

The inplace array is indeed somewhat tricky :) Here is one idea to make the effort easier for more similar cases: apology for typing in a window. Have fun!

// Curiously recurring template pattern
template<typename ArrayType, typename ElemType>
class InplaceArrayBase {
   public:
      // caclculate start location
      static_assert(sizeof(ArrayType) % align_of(ElemType));
     void InitEmpty(begin, end) {
          CHECK(i < self->capacity());
          for (i =begin; i< end; ++i) { 
            void* ptr = AddressOf(i);
            new (ptr) ElemType();
         }
     }  
    // other initializers, please think a bit more carefully about the API
    ~InplaceArrayBase() {
         ArrayType* self = static_cast<ArrayType*>(this);
         if (!std::is_pod<ElemType>::value) {
           // destruct elements
            for (size_t i = 0; i < self->size(); ++i) {
               void* ptr = AddressOf(i);
               reinterpret_cast<ElemType*>(ptr)->ElemType::~ElemType();
            }
         }
      } 
      ElemType& operator[](size_t i) {
         ArrayType* self = static_cast<ArrayType*>(this);

            void* ptr = AddressOf(i);
         return *reinterpret_cast<ElemType*>(ptr);
      }
   private:
     static constexpr const int kDataStart = sizeof(ArrayType);
     void* AddressOf(int i) {
        ArrayType* self = static_cast<ArrayType*>(this);
        return reinterpret_cast<char*>(self) + kDataStart;  
     }
};

class ADTObj : public Object, public InplaceArrayBase<ADTObj, ObjectRef> {
}

[tqchen]

By aligned storage, I mean we use aligned storage to specify the unit size, and allocate an array of aligned storage, whose number of elements can be passed in runtime

[wweic]

By aligned storage, I mean we use aligned storage to specify the unit size, and allocate an array of aligned storage, whose number of elements can be passed in runtime

Thanks @tqchen. I missed the array part. That makes sense. I'm going to create a new handler in memory.h probably named ArrayHandler(suggestions welcome) which handles types like this(a header type followed by an array of element types). I'll also improve the user facing interface of ADTObject. (standard container like APIs and handle lifecycle correctly).

[wweic]

Comments addressed. Please take another look. @tqchen @zhiics @icemelon9 @junrushao1994

[junrushao]

Will do tomorrow :-)

[wweic]

@tqchen The c++ compiler in CI does not support defining static constexpr from sizeof of ADTObj, since its definition is not complete. I saw the error last time so I changed it to runtime constant. Do you want to upgrade the compiler version or I'll change it back to runtime constant?

/workspace/include/tvm/runtime/container.h:138:40: 
error: invalid application of 'sizeof' to an incomplete type 'tvm::runtime::ADTObj'

  static constexpr size_t kDataStart = sizeof(ArrayType);

/workspace/include/tvm/runtime/container.h:163:7: note: 
definition of 'tvm::runtime::ADTObj' is not complete until the closing '}'

class ADTObj : public Object, public InplaceArrayBase<ADTObj, ObjectRef> {

[tqchen]

Interesting, sorry I didn't realize that due to injection the child class was not complete. It is fine to turn into runtime constant then, but we still need the static assert. An alternative way to do that is to introduce an auxiliary traits class that defines these constants and derived types, and then make use of these constants inside the functions(which will then know the complete type)

[tqchen]

Comments about Init function design, we want something that is minimum(can be reused by Vector style sub-class and Tuple-style sub-classes), while not interfering too much with low-level content.

One possible such interface could looks like http://www.cplusplus.com/reference/vector/vector/emplace_back/

protected:
  template <typename... Args>
  void EmplaceInit(size_t idx, Args&&... args);

Which EmplaceInit initializes elements at location idx(given the that idx has not been initialized before. This function can then be used by various other initializer/push_back functions(which we can debate whether they should be in the sub-class or base).

[wweic]

@tqchen I changed the interface a little bit. Now the set of interfaces are:

1. assign(initializer_list/iterator). This is the interface to initialize an array.
2. push_back(value). This is the interface to insert elements to the array with a value
3. emplace_back(args). This is the interface to insert elements to the array in place with arguments. It uses EmplaceInit under the hood.
   The reason to have 1 is that it's common to have a source array we want to copy from(ADT, Tuple, for example), we can not easily do inplace initialization. 2 and 3 achieves the same thing just how to do it.

[tqchen]

@wweic Some further comments. I think we don't have to implement push_back and emplace_back at the base level as ADT are immutable, we just need to have easy tools to initialize an element(EmplaceInit) and can build the rest of the features in the sub-class on top of the base class.

Thanks for the changes so far. Base data structure is a fundamental piece for the system, so we have to be extra picky in reviewing the code, but it is also rewarding and fun to build.

[wweic]

@tqchen Tests added. It's a bit contrived since I need to ensure deterministic exception during unit testing. Please take a look.

[tqchen]

Thanks @wweic @junrushao1994 , this pr is now merged

[tqchen]

@wweic seems that the change breaks MSVC compilation https://dev.azure.com/tvmai/tvm/_build/results?buildId=3440

please followup and send a fix

[wweic]

@tqchen Can we make PR check in blocked on Azure Build test pass as well?