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.

Sign up for GitHub

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

Jump to bottom

Add more size classes #10139

Closed
wants to merge 1 commit into from
Closed

Add more size classes #10139

wants to merge 1 commit into from

Conversation

oerling
Copy link
Contributor

@oerling oerling commented Jun 11, 2024

Experiment to see dynamics with 2, 4, 8, 16 MB classes added.

@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 Jun 11, 2024
@netlify Netlify
Copy link

netlify bot commented Jun 11, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 01dd2ff
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66749bf4cbaded0008054ae5

@facebook-github-bot
Copy link
Contributor

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@coco-dodo
Copy link

May I inquire about the motivation behind this modification? @oerling

We have an analytic workload that relies on a Velox kernel-based compute engine.
After integrating some third-party libraries that allocate memory using MmapAllocator, we noticed a high CPU sys load due to numerous mmap and unmap calls.

We suspect that the third-party library’s consecutive memory allocations exceed Velox MmapAllocator’s maximum size class of 1MB.
Additionally, we observed that we’ve disabled the managed mapped arena memory allocator option.

Is this PR addressing a similar issue?
Will introducing larger size classes cause memory fragmentation issues in the normalized path?

@oerling oerling force-pushed the add-sc-pr branch 2 times, most recently from 5a4a672 to ba9a38d Compare June 18, 2024 19:31
xiaoxmeng
xiaoxmeng approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@oerling LGTM. Thanks!

velox/common/memory/Memory.h
@@ -92,6 +92,9 @@ struct MemoryManagerOptions {
/// std::malloc.
bool useMmapAllocator{false};

// Number of pages in largest size class in MmapAllocator.
Copy link
Contributor

Choose a reason for hiding this comment

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

/// Number of pages ..

Add more size classes
01dd2ff 
Experiment to see dynamics with 2, 4, 8, 16 MB classes added.
@facebook-github-bot
Copy link
Contributor

@oerling has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@oerling merged this pull request in 1f981ae.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 1f981ae8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oerling @facebook-github-bot @coco-dodo @xiaoxmeng