Uh oh!
There was an error while loading. Please reload this page.
Avoid memory allocations and deallocations when creating NVTETensor - #1813
Conversation
Signed-off-by: Przemek Tredak <ptredak@nvidia.com>
Signed-off-by: Przemek Tredak <ptredak@nvidia.com>
for more information, see https://pre-commit.ci
ptrendx
commented
May 21, 2025
/te-ci |
Uh oh!
There was an error while loading. Please reload this page.
| // Should be replaced by virtual memory allocation | ||
| const size_t MAX_TENSOR_NUM = 20 * 1024 * 1024 / sizeof(Tensor); | ||
| std::vector<uintptr_t> free_list; | ||
| std::vector<Tensor> memory; |
There was a problem hiding this comment.
The fixed 20 MiB allocation size is a little hacky. How about storing the Tensors in unique_ptrs?
| std::vector<Tensor>memory; | |
| std::vector<std::unique_ptr<Tensor>> memory; |
This way the vector can freely reallocate and move unique_ptrs without disturbing the Tensors' addresses. We could also switch to a more appropriate data structure like unordered_set.
There was a problem hiding this comment.
I was thinking quite a lot about this actually before settling on that design. There are a few reasons why I chose it:
- with NVTETensor as a pointer (so that you can use the unordered_set) you don't have the ability to check if the parameter given to the function is actually a valid Tensor. Bugs in user code could then lead to crashes/security vulneabilities.
- in the regular usage the NVTETensor values used in a function would be very close together, helping with the memory access pattern (that would be best if we got rid of the std::vector for the shape in Tensor and instead used NVTEShape, making Tensor fully POD. Keep in mind we are fighting for microseconds here.
- I would argue that the 20MB limit is actually way too high. It enables creating over 83k Tensors. There is no real way of getting to that number without memory leak.
There was a problem hiding this comment.
- Making
NVTETensoran index for security reasons seems sensible. I would suggest keeping this PR's index logic, but storing the tensors themselves inunique_ptrs. - In most cases,
unique_ptrshould compile down to the same instructions as raw pointers. The only performance impact should be with caching. - If cache misses become performance critical, we could allocate
Tensors in blocks of a few cache lines. This would result in identical performance as a single contiguous block, but with an adaptive buffer size. - I don't think we should strive to make
Tensora full POD. It would benefit from a redesign for polymorphism (see the visitor pattern prototype in this branch).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Przemek Tredak <ptredak@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Przemek Tredak <ptredak@nvidia.com>
for more information, see https://pre-commit.ci
Signed-off-by: Przemek Tredak <ptredak@nvidia.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: Przemek Tredak <ptredak@nvidia.com>
ptrendx
commented
May 28, 2025
/te-ci |
ptrendx
commented
May 28, 2025
/te-ci |
timmoon10
left a comment
There was a problem hiding this comment.
Overall LGTM. I still don't like that we allocate Tensors out of a fixed 20 MiB buffer, but it's an implementation detail that's not exposed externally.
Description
Changed the transformer_engine::Tensor allocation to use static pool. NVTETensor is no longer a pointer to Tensor, but rather an index into the pool.
Type of change
Changes
Please list the changes introduced in this PR:
reinterpret_cast<Tensor*>logic for converting NVTETensors to transformer_engine::Tensor to call a dedicated conversion function instead.Checklist: