Skip to content
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

Don't reallocate buffers in fragment consolidation #4614

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

davisp
Copy link
Contributor

@davisp davisp commented Jan 10, 2024

Avoid needless buffer reallocations when consolidating fragments.


TYPE: IMPROVEMENT
DESC: Avoid needless buffer reallocations when consolidating fragments.

@davisp davisp force-pushed the pd/fragment_consolidation_workspace branch from e87bdeb to b5334e7 Compare January 10, 2024 18:49
Base automatically changed from eh/fragment_consolidation_workspace to dev January 10, 2024 18:52
@davisp
Copy link
Contributor Author

davisp commented Jan 10, 2024

Closing and re-opening from a different branch fro shortcut integration.

Apparently you can just tag a comment with the sc-##### in brackets and it links to the story.

@davisp davisp closed this Jan 10, 2024
@davisp davisp reopened this Jan 10, 2024
@davisp
Copy link
Contributor Author

davisp commented Jan 10, 2024

[sc-36372]

Copy link

@davisp davisp force-pushed the pd/fragment_consolidation_workspace branch from b5334e7 to 9f25912 Compare January 10, 2024 18:59
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

I'm not convinced that calling resize is better than destroying and recreating the workspace. If the sizes don't change (maybe for shortening), then sure, it'll be more efficient. On the other hand, lengthening will often require a memory copy because vector::resize preserves all the existing members before the operation.

vector does default-initialization (setting to zero) as well as copies on resize. Do we need either of these behaviors from the container? Would uninitialized memory work? The first thing we're doing is writing into them with query results. Is there some expectation that the buffer be initialized with zero to work correctly?

@davisp davisp force-pushed the pd/fragment_consolidation_workspace branch from 9f25912 to 14176de Compare January 11, 2024 18:37
@davisp davisp marked this pull request as ready for review January 11, 2024 18:37
@davisp davisp force-pushed the pd/fragment_consolidation_workspace branch 2 times, most recently from ff75159 to d1fa68f Compare January 12, 2024 21:07
@KiterLuc KiterLuc force-pushed the pd/fragment_consolidation_workspace branch from d1fa68f to 5746668 Compare January 12, 2024 21:51
This is based on the work from @Shelnutt2 on branch `ss/sc-36372`.
@KiterLuc KiterLuc force-pushed the pd/fragment_consolidation_workspace branch from 5746668 to 2c5cbbe Compare January 12, 2024 22:02
@KiterLuc KiterLuc merged commit 7902e8b into dev Jan 13, 2024
62 checks passed
@KiterLuc KiterLuc deleted the pd/fragment_consolidation_workspace branch January 13, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants