-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate std::prm memory tracking for class Enumeration
.
#4735
Integrate std::prm memory tracking for class Enumeration
.
#4735
Conversation
- Deleted default constructor of class tiledb::sm::ArraySchema - Changed copy_with_new_memory_tracker to clone - Deleted constructor with pmr vector. Problems remaining: - had to make copy constructor public since a protected one does not work with our version of make_shared.
This pull request has been linked to Shortcut Story #40878: Integrate std::prm memory tracking for class |
e29d57d
to
f904b16
Compare
841f930
to
e80e797
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor changes I spotted on a quick skim. Same as for the ArraySchema PR, don't feel like you have to make those argument ordering changes yourself if you don't feel like it.
f904b16
to
e2ac764
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit. Other than that I'll approve this as soon as the ArrarySchema PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ope, I caught one last thing. When we're loading enumerations normally through the Array class APIs, we're inadvertently using the REST Client's MemoryTracker instead of the Array tracker. For this function:
TileDB/tiledb/sm/rest/rest_client.h
Lines 252 to 257 in 9883b34
std::vector<shared_ptr<const Enumeration>> post_enumerations_from_rest( | |
const URI& uri, | |
uint64_t timestamp_start, | |
uint64_t timestamp_end, | |
Array* array, | |
const std::vector<std::string>& enumeration_names); |
std::vector<shared_ptr<const Enumeration>> post_enumerations_from_rest(
const URI& uri,
uint64_t timestamp_start,
uint64_t timestamp_end,
Array* array,
const std::vector<std::string>& enumeration_names,
shared_ptr<MemoryTracker> memory_tracker = nullptr);
Then in the body of that function we can just do:
if (!memory_tracker) {
memory_tracker = memory_tracker_;
}
That way it won't affect anything other than where we specify which MemoryTracker at the call site.
Then we just have to add the memory_tracker_
argument here:
https://github.com/TileDB-Inc/TileDB/blob/dev/tiledb/sm/array/array.cc#L591-L596
Fixed this comment! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
…nc#4735) All member vector variables of the Enumeration class have PMR tracking. --- TYPE: NO_HISTORY DESC: Integrate std::prm memory tracking for class `Enumeration`. --------- Co-authored-by: Luc Rancourt <[email protected]>
All member vector variables of the Enumeration class have PMR tracking.
TYPE: NO_HISTORY
DESC: Integrate std::prm memory tracking for class
Enumeration
.