Add memory tracking for RTree ranges. #5029
Conversation
This automatically flows the allocator downwards from the root `levels_` vector.
| for (unsigned d = 0; d < dim_num; ++d) { | ||
| auto dim{domain->dimension_ptr(d)}; | ||
| if (!dim->var_size()) { // Fixed-sized | ||
| auto r_size = 2 * dim->coord_size(); | ||
| auto data = deserializer.get_ptr<void>(r_size); | ||
| levels_[l][m][d] = Range(data, r_size); | ||
| // NDRange is not yet a pmr vector, so we need to explicitly pass the |
There was a problem hiding this comment.
Making NDRange a tdb::pmr::pmr_vector would require changing many files that use std::vector<Range> to use NDRange. I can change them, but am wary about expanding the PR's scope.
There was a problem hiding this comment.
Let's file a follow up story here.
tiledb/type/range/range.h
Outdated
| @@ -61,14 +62,28 @@ class Range { | |||
| * The range is stored as a sequence of bytes with manual memory layout. The | |||
| * memory layout is different for fixed-size and variable-size types. | |||
| * | |||
| * The type is tdb::pmr::pmr_vector<uint8_t> instead of | |||
There was a problem hiding this comment.
@davisp Are there any reasons we disabled the copy constructors in tdb::pmr::vector? Are we worried the copy constructor that doesn't take a memory tracker will not do the right thing and we might have some allocator mixup or are we worries that someone can initialize a vector that we want tracked with a vector that's not tracked? If it's the later, could we throw in the copy constructor if the vector passed in doesn't have an allocator?
There was a problem hiding this comment.
There are two copy constructors. One takes an allocator and one doesn't. The one that takes an allocator is available for use.
The copy constructor that doesn't take an allocator is disabled. That choice was so that as we changed from std::vector to std::pmr::vector, we'd not miss any place where we used copy constructors because the compiler would catch the issue and force us to update those call sites with the allocator argument.
There was a problem hiding this comment.
- I restored the move constructor and move-assign operator in
tdb::pmr::vector. - I manually implemented the copy constructor and copy-assign operator in
Rangeto avoid breaking other uses.
teo-tsirpanis
force-pushed
the
teo/rtree-pmr
branch
from
942223a
to
f56b2c6
Compare
…ymorphic allocator.
They use those on vector.
| for (unsigned d = 0; d < dim_num; ++d) { | ||
| auto dim{domain->dimension_ptr(d)}; | ||
| if (!dim->var_size()) { // Fixed-sized | ||
| auto r_size = 2 * dim->coord_size(); | ||
| auto data = deserializer.get_ptr<void>(r_size); | ||
| levels_[l][m][d] = Range(data, r_size); | ||
| // NDRange is not yet a pmr vector, so we need to explicitly pass the |
There was a problem hiding this comment.
Let's file a follow up story here.
| : pmr_vector<Tp>(init, alloc) { | ||
| } | ||
| }; | ||
| using vector = std::vector<Tp, polymorphic_allocator<Tp>>; |
There was a problem hiding this comment.
Yes that is the change I think I want to make... Let me ponder on it for a little while longer. This is blocked on me now.
SC-46801
This PR enables memory tracking for RTree ranges. The first part consists of updating the range class to be backed by a
tdb::pmr::pmr_vector1 instead of anstd::vectorwhich allows users to specify a custom allocator that tracks memory, in an opt-in basis. The second part is updating the RTree class to use these allocators when creating the ranges.Validated locally. Let me know what kind of testing to add.
TYPE: NO_HISTORY
Footnotes
We are not using
tdb::pmr::vector, because it is not copy or move constructible, which would break other uses of ranges. Only ranges created by RTrees will be tracked in this PR. ↩