[RUNTIME][REFACTOR] Use new to avoid exit-time de-allocation order

[tqchen]

It is hard to control deallocation order of TVM and other frameworks. So just keep the global state active and rely on OS to recycle the resources

[tqchen]

cc @junrushao1994 @samskalicky @zhiics @yzhliu @jroesch @tmoreau89

[junrushao]

LGTM. Ideally all singletons should be created this way.

[tmoreau89]

LGTM

[samskalicky]

Thanks @tqchen! I will try building this into my code tonight and get back to you tomorrow

[samskalicky]

This still doesnt fix my problem in #5986. But I think its going in the right direction. Now we have a long-lived device object that wont be destroyed. But we cannot access it anymore after the DeviceAPIManager and its child DeviceAPI objects have already been deleted in the global scope when attempting to access from the NDArray deleter DefaultDeleter. In the deleter its calling tvm::runtime::DeviceAPI::Get to get the DeviceAPI pointer.

Instead, what if we store the DeviceAPI pointer in the NDArray during creation (or whenever we set/change the context) and then use that pointer inside the deleter? The actual DeviceAPI object on the heap will still be around, so that should work.

[tqchen]

@samskalicky In tihs case the deleter is already long-lived. If the deletion happens during the process of already unloading of vtable or other global objects, then there is little that we can do to solve the problem. Again, in the cases where multiple dlls are involved, the best way is to avoid retaining global objects in python.

[samskalicky]

@samskalicky In tihs case the deleter is already long-lived. If the deletion happens during the process of already unloading of vtable or other global objects, then there is little that we can do to solve the problem. Again, in the cases where multiple dlls are involved, the best way is to avoid retaining global objects in python.

Right, the problem could be that the DeviceAPIManager and its child DeviceAPI objects are shorter lived than the deleter. If we remove the dependency from the deleter to get the DeviceAPI through the (potentially destructed) DeviceAPIManager then the deleter doesnt depend on the lifetime of the DeviceAPIManager since it already has a pointer to the heap allocated DeviceAPI object.

[samskalicky]

Since this is definitely something out of our control (shutdown order, varies depending on application usage) and we've decided to rely on the process exiting to free any memory, can we consider clearing the pointers in DeviceAPIManager like:

  // destructor
  ~DeviceAPIManager() { std::fill(api_.begin(), api_.end(), nullptr); }

And then just make sure that wherever we try and get the DeviceAPI that we check to see if the pointer is null? Then we just skip freeing any memory and dont have to worry about objects destructed out of order

[tqchen]

NOTE that right now the DeviceAPIManager singleton is long-lived now, the value won't be cleared. Even if we choose to instead clear these values, there is no guarantee wrt to the order. So the best way is still to avoid global object in the code(if there are, avoid deleting them, like in this PR)

[samskalicky]

NOTE that right now the DeviceAPIManager singleton is long-lived now, the value won't be cleared. Even if we choose to instead clear these values, there is no guarantee wrt to the order. So the best way is still to avoid global object in the code(if there are, avoid deleting them, like in this PR)

Agreed, but thats not always possible given other framework requirements/architecture. Shouldnt we at least be able to guarantee an orderly shutdown for TVM components (runtime/ndarrays/etc) regardless?

[tqchen]

To be clear, I don't think it is the problem of shutdown order(as the objects are now long-lived). It seems that the function is called after the dll get unloaded, so there is little we can do.

[kparzysz-quic]

This will not invoke any destructors on exit! What problem is this trying to solve?

[tqchen]

@kparzysz-quic The main problem it tries to resolve is to avoid the problems that can be caused by differnt order destruction of static objects.

For example, if a static object takes NDArray, and it get destructed after the DeviceAPI get destructed, then there will be an error.

The resources(e.g. in the case of memory etc) will be recycled as OS recycles the process after unloading.

[kparzysz-quic]

This will likely cause errors with address sanitizer.

We really need a better solution in the long term.

[kparzysz-quic]

I'm not against it as a temporary solution.

[tqchen]

I see, not freeing the static global singleton is a common pattern though. Is it possible to somehow provide hints to ASAN to disable the warnings?

[kparzysz-quic]

There is a way to turn off the leak sanitizer: define a function int __lsan_is_turned_off(void); and make it return non-zero.

Here's the description (from compiler-rt/include/sanitizer/lsan_interface.h):

  // The user may optionally provide this function to disallow leak checking
  // for the program it is linked into (if the return value is non-zero). This
  // function must be defined as returning a constant value; any behavior beyond
  // that is unsupported.
  // To avoid dead stripping, you may need to define this function with
  // __attribute__((used))
  int __lsan_is_turned_off(void);

Btw, for static singleton, the C++ compiler will generate calls to their destructors when main terminates.

[kparzysz-quic]

This is how the leak-suppression function is used in one of the source files in LLVM (in TableGen utility):

#ifndef __has_feature
#define __has_feature(x) 0
#endif

#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__) ||       \
    __has_feature(leak_sanitizer)

#include <sanitizer/lsan_interface.h>
// Disable LeakSanitizer for this binary as it has too many leaks that are not
// very interesting to fix. See compiler-rt/include/sanitizer/lsan_interface.h .
LLVM_ATTRIBUTE_USED int __lsan_is_turned_off() { return 1; }

#endif

[kparzysz-quic]

Sorry, tried to clear the comment window and hit the wrong button. :(

[tqchen]

OK, seems the concensus is to keep this solution for now. Would be great to have more conversations. @kparzysz-quic no worries, glad to see that you now have the committer perm setup correctly :)