Use new memory tracker for tile offsets #4929
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KiterLuc
reviewed
May 3, 2024
KiterLuc
reviewed
May 3, 2024
KiterLuc
reviewed
May 3, 2024
| @@ -253,8 +253,7 @@ void SparseUnorderedWithDupsReader<BitmapType>::load_tile_offsets_data() { | |||
| tile_offsets_min_frag_idx_ == std::numeric_limits<unsigned>::max() && | |||
| tile_offsets_max_frag_idx_ == 0; | |||
| uint64_t available_memory = | |||
| array_memory_tracker_->get_memory_available() - | |||
| array_memory_tracker_->get_memory_usage(MemoryType::TILE_OFFSETS); | |||
| array_memory_tracker_->get_memory_available(); | |||
There was a problem hiding this comment.
get_memory_available should be changed so that tile offsets from the new counters are subtracted.
There was a problem hiding this comment.
I have done that.
DimitrisStaratzis
force-pushed
the
dstara/use_new_memory_tracker_for_offsets
branch
2 times, most recently
from
72de436
to
2441e3d
Compare
DimitrisStaratzis
changed the title
DimitrisStaratzis
changed the title
DimitrisStaratzis
force-pushed
the
dstara/use_new_memory_tracker_for_offsets
branch
3 times, most recently
from
4afb631
to
809c0bf
Compare
DimitrisStaratzis
force-pushed
the
dstara/use_new_memory_tracker_for_offsets
branch
2 times, most recently
from
7ca6d12
to
14e35a7
Compare
DimitrisStaratzis
force-pushed
the
dstara/use_new_memory_tracker_for_offsets
branch
from
a18b159
to
44d601e
Compare
KiterLuc
reviewed
May 17, 2024
| @@ -312,7 +312,10 @@ class MemoryTracker { | |||
| */ | |||
| uint64_t get_memory_available() { | |||
| std::lock_guard<std::mutex> lg(mutex_); | |||
| return memory_budget_ - memory_usage_; | |||
| if (memory_usage_ + counters_[MemoryType::TILE_OFFSETS] > memory_budget_) { | |||
| // Finally set the size of the loaded data. | ||
| ret[frag_idx] = num * tile_num * sizeof(uint64_t); | ||
| ret[frag_idx] = |
There was a problem hiding this comment.
We need to explain the 4 and 32.
| unsigned num_vectors = schema->attribute_num() + 1 + | ||
| fragment->has_timestamps() + | ||
| fragment->has_delete_meta() * 2; | ||
| num_vectors += (fragment->version() >= 5) ? schema->dim_num() : 0; |
There was a problem hiding this comment.
Add a comment on this line as to what it does... For fragments up to version 4, we use zipped coordinates.
| // Other than the offsets themselves, there is also memory used for the | ||
| // initialization of the vectors that hold them. This initialization | ||
| // takes place in fragment_metadata.cc::load_footer() | ||
| unsigned num_vectors = schema->attribute_num() + 1 + |
There was a problem hiding this comment.
Document the +1. I think it's the old zipped coords?
There was a problem hiding this comment.
This is the only thing I don't understand. What this +1 stands for. It is documented nowhere
| // Other than the offsets themselves, there is also memory used for the | ||
| // initialization of the vectors that hold them. This initialization | ||
| // takes place in fragment_metadata.cc::load_footer() | ||
| unsigned num_vectors = schema->attribute_num() + 1 + |
There was a problem hiding this comment.
Suggested change
| unsigned num_vectors = schema->attribute_num() + 1 + | |
| unsigned num_fields = schema->attribute_num() + 1 + |
KiterLuc
approved these changes
May 17, 2024
KiterLuc
pushed a commit
that referenced
this pull request
May 27, 2024
Fixes the nightly tests in windows after #4929. The memory budgets tests were failing because structures take more memory in ASAN debug. Adjusting the budgeting values so they pass in all build flavors. Successful run here: https://github.com/TileDB-Inc/TileDB/actions/runs/9209028209. --- TYPE: NO_HISTORY DESC: Update budget tests to fix windows nightly builds.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is responsible for two things:
[sc-46275]
TYPE: NO_HISTORY
DESC: Use new memory tracker for tile offsets.