You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/06/19 07:31:47 UTC
[GitHub] [tvm] ganler opened a new pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
ganler opened a new pull request #8285:
URL: https://github.com/apache/tvm/pull/8285
See [TVM Discussion Topic](https://discuss.tvm.apache.org/t/logfatal-may-skip-some-important-errors-exceptions/10281
).
This change aims to make TVM behaviour more robust when an OOM occurs and resolve a mysterious exception-uncaught bug.
Potential reviewers: @junrushao1994 @icemelon9 @jroesch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654876839
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Fair enough. I don't have other comments then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
ganler commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654871849
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Thanks for the suggestion. But IMHO this is not robust enough.
Say that we have 8 GB GPU memory, the pooled cached 4 GB and we want to allocate 6 GB.
- Applying your idea, `ReleaseAll()` returns "4GB" which is less than "6GB", thus resulting in a failed allocation.
- Instead, if we release unused memory and do re-allocation, "6GB" is very likely to be successfully allocated.
The big picture behind your idea is practical if we can have some APIs like "total_system_memory" and "available_system_memory", which may require introducing a series of runtime/driver libraries. e.g., `cudaMemGetInfo` by CudaRT (user space) or `NVML` (if some system privilege is allowed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654832904
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
What to expect if it still failed here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
ganler commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654871849
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Thanks for the suggestion. But IMHO this is not robust enough.
Say that we have 8 GB GPU memory, the pooled cached 4 GB and we want to allocate 6 GB.
- Applying your idea, `ReleaseAll()` returns "4GB" which is less than "6GB", thus resulting in a failed allocation.
- Instead, if we release unused memory and do re-allocation, "6GB" is very likely to be successfully allocated.
But the big picture behind your idea is practical if we can have some APIs like "total_system_memory" and "available_system_memory", which may require introducing a series of runtime/driver libraries. e.g., `cudaMemGetInfo` by CudaRT (user space) or `NVML` (more system privilege is allowed).
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Thanks for the suggestion. But IMHO this is not robust enough.
Say that we have 8 GB GPU memory, the pooled cached 4 GB and we want to allocate 6 GB.
- Applying your idea, `ReleaseAll()` returns "4GB" which is less than "6GB", thus resulting in a failed allocation.
- Instead, if we release unused memory and do re-allocation, "6GB" is very likely to be successfully allocated.
The big picture behind your idea is practical if we can have some APIs like "total_system_memory" and "available_system_memory", which may require introducing a series of runtime/driver libraries. e.g., `cudaMemGetInfo` by CudaRT (user space) or `NVML` (more system privilege is allowed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
ganler commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654864035
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
If it still fails, an InternalError will be thrown, causing a TVMError regarding OOM in the Python End.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] YuchenJin commented on pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
YuchenJin commented on pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#issuecomment-864451198
LGTM! Thanks @ganler for the fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] YuchenJin removed a comment on pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
YuchenJin removed a comment on pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#issuecomment-864451198
LGTM! Thanks @ganler for the fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jcf94 merged pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
jcf94 merged pull request #8285:
URL: https://github.com/apache/tvm/pull/8285
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
ganler commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654871849
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Thanks for the suggestion. But IMHO this is not robust enough.
Say that we have 8 GB GPU memory, the pooled cached 4 GB and we want to allocate 6 GB.
- Applying your idea, `ReleaseAll()` returns "4GB" which is less than "6GB", thus resulting in a failed allocation.
- Instead, if we release unused memory and do re-allocation, "6GB" is very likely to be successfully allocated.
But the big picture behind your idea is practical if we can have some APIs like "total_system_memory" and "available_system_memory", which may require introducing a series of system driver libraries. e.g., `cudaMemGetInfo` by CudaRT (user space) or `NVML` (more system privilege is allowed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654869776
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Hmm, would that be better if we let ReleaseAll return the size it released and check if it is larger than the requested size? So that we can directly throw a message including both sizes without calling alloc again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ganler commented on a change in pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
ganler commented on a change in pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#discussion_r654871849
##########
File path: src/runtime/vm/pooled_allocator.h
##########
@@ -57,14 +57,22 @@ class PooledAllocator final : public Allocator {
Buffer buf;
buf.device = device_;
buf.size = size;
- buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ try {
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
+ } catch (InternalError& err) {
+ LOG(WARNING) << "PooledAllocator got InternalError during allocation: " << err.message();
+ LOG(WARNING) << "Trying to release all unused memory and reallocate...";
+ ReleaseAll();
+ buf.data = DeviceAPI::Get(device_)->AllocDataSpace(device_, size, alignment, type_hint);
Review comment:
Thanks for the suggestion. But IMHO this is not robust enough.
Say that we have 8 GB GPU memory, the `PooledAllocator` cached 4 GB and we want to allocate 6 GB.
- Applying your idea, `ReleaseAll()` returns "4GB" which is less than "6GB", thus resulting in a failed allocation.
- Instead, if we release unused memory and do re-allocation, "6GB" is very likely to be successfully allocated.
The big picture behind your idea is practical if we can have some APIs like "total_system_memory" and "available_system_memory", which may require introducing a series of runtime/driver libraries. e.g., `cudaMemGetInfo` by CudaRT (user space) or `NVML` (if some system privilege is allowed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #8285: [VM][PooledAllocator] try reallocation once when OOM
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8285:
URL: https://github.com/apache/tvm/pull/8285#issuecomment-903641187
This change doesn't solve the issue in https://github.com/apache/tvm/issues/8233, because `AllocDataSpace` can be called from `NDArray::Empty`: https://github.com/apache/tvm/blob/4d9bc9b4a3e9e8d3420efe60a52964fcd4c29c8d/src/runtime/ndarray.cc#L196-L197
That call is not protected by try/catch, so if almost all memory are held by `PooledAllocator` and `NDArray::Empty` is called, the program crashes with the following error:
```
terminate called after throwing an instance of 'tvm::runtime::InternalError'
what(): [19:12:54] /home/masa/projects/dev/tvm/src/runtime/vulkan/vulkan_stream.cc:123:
---------------------------------------------------------------
An error occurred during the execution of TVM.
For more information, please see: https://tvm.apache.org/docs/errors.html
---------------------------------------------------------------
Check failed: (__e == VK_SUCCESS) is false: Vulkan Error, code=-13: Unknown Vulkan error code
Stack trace:
0: tvm::runtime::vulkan::VulkanStream::Synchronize()
1: _ZN3tvm7runtime6vulkan15VulkanDeviceAPI13FreeDataSpac
2: tvm::runtime::NDArray::Internal::DefaultDeleter(tvm::runtime::Object*)
3: tvm::runtime::NDArray::CopyTo(DLDevice const&) const
4: tvm::runtime::vm::CopyTo(tvm::runtime::ObjectRef, DLDevice const&)
5: std::_Function_handler<void (tvm::runtime::TVMArgs, tvm::runtime::TVMRetValue*), tvm::runtime::vm::VirtualMachine::GetFunction(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, tvm::runtime::ObjectPtr<tvm::runtime::Object> const&)::$_6>::_M_invoke(std::_Any_data const&, tvm::runtime::TVMArgs&&, tvm::runtime::TVMRetValue*&&)
6: TVMFuncCall
```
I think we need to revisit the memory release strategy of `PooledAllocator`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org