Fix issue #793: Implement slab based tape - #1404
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| class tape_impl { | ||
| T* _data = nullptr; | ||
| struct Slab { | ||
| alignas(T) char raw_data[32 * sizeof(T)]; |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
alignas(T) char raw_data[32 * sizeof(T)];
^| alignas(T) char raw_data[32 * sizeof(T)]; | ||
| Slab* next; | ||
| Slab() : next(nullptr) {} | ||
| T* elements() { return reinterpret_cast<T*>(raw_data); } |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
T* elements() { return reinterpret_cast<T*>(raw_data); }
^| } | ||
| Slab* tmp = slab; | ||
| slab = slab->next; | ||
| delete tmp; |
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete tmp;
^Additional context
include/clad/Differentiator/Tape.h:133: variable declared here
Slab* tmp = slab;
^
| size_t i = _size / slab_size; | ||
| while (i--) slab = slab->next; | ||
|
|
||
| ::new (const_cast<void*>(static_cast<const volatile void*>(slab->elements() + (_size % slab_size)))) T(std::forward<ArgsT>(args)...); |
There was a problem hiding this comment.
warning: do not use const_cast to remove const and volatile qualifier [cppcoreguidelines-pro-type-const-cast]
::new (const_cast<void*>(static_cast<const volatile void*>(slab->elements() + (_size % slab_size)))) T(std::forward<ArgsT>(args)...);
^There was a problem hiding this comment.
There must be some idiom that's used in STL for such operations instead of these casts...
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| class tape_impl { | ||
| T* _data = nullptr; | ||
| struct Slab { | ||
| alignas(T) char raw_data[1024 * sizeof(T)]; |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
alignas(T) char raw_data[1024 * sizeof(T)];
^| constexpr static std::size_t slab_size = 1024; | ||
|
|
||
| CUDA_HOST_DEVICE T* sbo_elements() { | ||
| return reinterpret_cast<T*>(static_buffer_); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
return reinterpret_cast<T*>(static_buffer_);
^| } | ||
|
|
||
| CUDA_HOST_DEVICE const T* sbo_elements() const { | ||
| return reinterpret_cast<const T*>(static_buffer_); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
return reinterpret_cast<const T*>(static_buffer_);
^| T(std::forward<ArgsT>(args)...); | ||
| _size += 1; | ||
| if (_size < SBO_SIZE) { | ||
| ::new (const_cast<void*>(static_cast<const volatile void*>(sbo_elements() + _size))) |
There was a problem hiding this comment.
warning: do not use const_cast to remove const and volatile qualifier [cppcoreguidelines-pro-type-const-cast]
::new (const_cast<void*>(static_cast<const volatile void*>(sbo_elements() + _size)))
^| using_sbo_ = false; | ||
| } | ||
| if ((_size - SBO_SIZE) % slab_size == 0) { | ||
| Slab* new_slab = new Slab(); |
There was a problem hiding this comment.
warning: initializing non-owner 'Slab *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
Slab* new_slab = new Slab();
^
| class tape_impl { | ||
| T* _data = nullptr; | ||
| struct Slab { | ||
| alignas(T) char raw_data[1024 * sizeof(T)]; |
There was a problem hiding this comment.
Can you also test with bigger objects where sizeof(T) influences the overall size?
| T* _data = nullptr; | ||
| /// A block of contiguous storage allocated dynamically when SBO capacity is exceeded. | ||
| struct Slab { | ||
| alignas(T) char raw_data[SLAB_SIZE * sizeof(T)]{}; |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
alignas(T) char raw_data[SLAB_SIZE * sizeof(T)]{};
^| alignas(T) char raw_data[SLAB_SIZE * sizeof(T)]{}; | ||
| Slab* next; | ||
| CUDA_HOST_DEVICE Slab() : next(nullptr) {} | ||
| CUDA_HOST_DEVICE T* elements() { return reinterpret_cast<T*>(raw_data); } |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
CUDA_HOST_DEVICE T* elements() { return reinterpret_cast<T*>(raw_data); }
^| CUDA_HOST_DEVICE T* elements() { return reinterpret_cast<T*>(raw_data); } | ||
| }; | ||
|
|
||
| alignas(T) char static_buffer[SBO_SIZE * sizeof(T)]; |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
alignas(T) char static_buffer[SBO_SIZE * sizeof(T)];
^| std::size_t _capacity = 0; | ||
|
|
||
| CUDA_HOST_DEVICE T* sbo_elements() { | ||
| return reinterpret_cast<T*>(static_buffer); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
return reinterpret_cast<T*>(static_buffer);
^| } | ||
|
|
||
| CUDA_HOST_DEVICE const T* sbo_elements() const { | ||
| return reinterpret_cast<const T*>(static_buffer); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
return reinterpret_cast<const T*>(static_buffer);
^| while (idx--) slab = slab->next; | ||
|
|
||
| // Construct element in-place | ||
| ::new (const_cast<void*>(static_cast<const volatile void*>( |
There was a problem hiding this comment.
warning: do not use const_cast to remove const and volatile qualifier [cppcoreguidelines-pro-type-const-cast]
::new (const_cast<void*>(static_cast<const volatile void*>(
^
| CUDA_HOST_DEVICE T* elements() { return reinterpret_cast<T*>(raw_data); } | ||
| }; | ||
|
|
||
| alignas(T) char m_static_buffer[SBO_SIZE * sizeof(T)]; |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
alignas(T) char m_static_buffer[SBO_SIZE * sizeof(T)];
^| std::size_t _capacity = 0; | ||
|
|
||
| CUDA_HOST_DEVICE T* sbo_elements() { | ||
| return reinterpret_cast<T*>(m_static_buffer); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
return reinterpret_cast<T*>(m_static_buffer);
^| } | ||
|
|
||
| CUDA_HOST_DEVICE const T* sbo_elements() const { | ||
| return reinterpret_cast<const T*>(m_static_buffer); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
return reinterpret_cast<const T*>(m_static_buffer);
^| } | ||
| Slab* tmp = slab; | ||
| slab = slab->next; | ||
| delete tmp; |
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete tmp;
^Additional context
include/clad/Differentiator/Tape.h:194: variable declared here
Slab* tmp = slab;
^
1f04a4b to
5332c48
Compare
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
We are getting very close to merging this.
5332c48 to
82ab54a
Compare
82ab54a to
b9fd321
Compare
b9fd321 to
1668252
Compare
| (elems + i)->~T(); | ||
| Slab* tmp = slab; | ||
| slab = slab->next; | ||
| delete tmp; |
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete tmp;
^Additional context
include/clad/Differentiator/Tape.h:262: variable declared here
Slab* tmp = slab;
^
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
LGMT! Gread job, @aditimjoshi!
1668252 to
cd92024
Compare
cd92024 to
aa73be8
Compare
3 participants
Fixes #793. Modifies tape implementation to a slab based structure instead of relocating elements each time the tape grows in size.