Skip to content
This repository has been archived by the owner on May 9, 2024. It is now read-only.

[Fetch Data] Linearized buffer memory managment #679

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Devjiu
Copy link
Contributor

@Devjiu Devjiu commented Sep 27, 2023

This commit removes freeLinearizedBuf() to avoid chunks that have
references to deleted buffers. They will now be marked as deleted when
unpinned. All buffers should now be removed via BufferManagers::free.

Signed-off-by: Dmitrii Makarenko dmitrii.makarenko@intel.com

@Devjiu Devjiu changed the title [Join] Buffer free [Fetch Data] Linearized buffer memory managment Sep 28, 2023
@@ -478,6 +481,18 @@ const int8_t* ColumnFetcher::linearizeColumnFragments(
// prepare ChunkIter for the linearized chunk
auto merged_chunk = std::make_shared<Chunk_NS::Chunk>(
merged_data_buffer, merged_index_buffer, col_info);
merged_data_buffer->deleteWhenUnpinned();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's enough to call this method once (in dtor).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, review again, please

@Devjiu Devjiu marked this pull request as ready for review October 2, 2023 14:30
@Devjiu Devjiu force-pushed the dmitriim/buffer_fix branch 2 times, most recently from af91d3a to 9070957 Compare October 10, 2023 13:47
This commit removes freeLinearizedBuf() to avoid chunks that have
references to deleted buffers. They will now be marked as deleted when
unpinned. All buffers should now be removed via BufferManagers::free.

Signed-off-by: Dmitrii Makarenko <dmitrii.makarenko@intel.com>
@Devjiu
Copy link
Contributor Author

Devjiu commented Oct 11, 2023

@alexbaden @kurapov-peter PTAL, this pr changes free method contract a little, and also changes interaction of buffer with bufferManager. It works, but perhaps you have some recommendations about method's visibility and class responsibilities.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants