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

misc(memory) Allow exact buffer allocation #12594

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

Conversation

joeg
Copy link

@joeg joeg commented Mar 10, 2025

Summary:
By default buffers are allocated up to 50% headroom :

size_t MemoryPool::preferredSize(size_t size) {

In the simple tpch demo this reduced overall data sizes by about ~6% - this scales to encoded vectors at the moment (dictionaries) and there is likely an equally large win for flat vectors given they are copied (vector copy also uses this same default buffer allocation logic).

For scenarios that need to maintain long lived data this is wasteful, Enabling an optional exact allocation (size + padding) reduces this waste.

Reviewed By: xiaoxmeng

Differential Revision: D70521769

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 10, 2025
Copy link

netlify bot commented Mar 10, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 430b276
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67d07ff96aff760008142b53

@joeg joeg force-pushed the export-D70521769 branch from 6bf01bc to f455819 Compare March 10, 2025 22:21
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70521769

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70521769

@joeg joeg force-pushed the export-D70521769 branch from f455819 to 8a4798d Compare March 10, 2025 22:25
@joeg joeg changed the title Allow exact buffer allocation [misc] Allow exact buffer allocation Mar 11, 2025
@joeg joeg changed the title [misc] Allow exact buffer allocation misc(memory) Allow exact buffer allocation Mar 11, 2025
@joeg joeg force-pushed the export-D70521769 branch from 8a4798d to ce3c41c Compare March 11, 2025 00:17
joeg added a commit to joeg/velox that referenced this pull request Mar 11, 2025
Summary:

By default buffers are allocated up to 50% headroom : https://www.internalfb.com/code/fbsource/[cbb63d0a88c6]/fbcode/velox/common/memory/MemoryPool.cpp?lines=397

In the simple tpch demo this reduced overall data sizes by about ~6% - this scales to encoded vectors at the moment (dictionaries) and there is likely an equally large win for flat vectors given they are copied (vector copy also uses this same default buffer allocation logic). 

For scenarios that need to maintain long lived data this is wasteful, Enabling an optional exact allocation (size + padding) reduces this waste.

Reviewed By: xiaoxmeng

Differential Revision: D70521769
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70521769

Summary:

By default buffers are allocated up to 50% headroom : https://www.internalfb.com/code/fbsource/[cbb63d0a88c6]/fbcode/velox/common/memory/MemoryPool.cpp?lines=397

In the simple tpch demo this reduced overall data sizes by about ~6% - this scales to encoded vectors at the moment (dictionaries) and there is likely an equally large win for flat vectors given they are copied (vector copy also uses this same default buffer allocation logic). 

For scenarios that need to maintain long lived data this is wasteful, Enabling an optional exact allocation (size + padding) reduces this waste.

Reviewed By: xiaoxmeng

Differential Revision: D70521769
@joeg joeg force-pushed the export-D70521769 branch from ce3c41c to 430b276 Compare March 11, 2025 18:24
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D70521769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants