Fix AllocationPoll::newRun fails when requests exceed largestSizeClass#4713
Closed
marin-ma wants to merge 5 commits intofacebookincubator:mainfrom
Closed
Fix AllocationPoll::newRun fails when requests exceed largestSizeClass#4713marin-ma wants to merge 5 commits intofacebookincubator:mainfrom
marin-ma wants to merge 5 commits intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
Collaborator
Author
xiaoxmeng
reviewed
Apr 23, 2023
| } | ||
| pool_->allocateNonContiguous( | ||
| std::max<int32_t>(kMinPages, numPages), allocation_, numPages); | ||
| std::max<int32_t>(kMinPages, numPages), allocation_); |
Contributor
There was a problem hiding this comment.
pool_->allocateNonContiguous(
std::max<int32_t>(kMinPages, numPages), allocation_, std::min(numPages, pool_->largestSizeClass()));
| } | ||
|
|
||
| TEST_P(MemoryAllocatorTest, exceedLargestSizeClass) { | ||
| const size_t kExceedLargestSizeClass = instance_->largestSizeClass() + 1; |
Contributor
There was a problem hiding this comment.
Can you test with different allocation size based on the class size and check the number of page runs in the resulting allocation?
AllocationPool::allocationAt(0)?
1, => expect one page run?
first size class + 1 => expect two page runs?
second size class + 1 => expect two page runs?
...
largestSizeClass() + 1 => expect two page runs?
You can structure the test with a while loop and each iteration test different size + expected page run? Thanks!
Collaborator
Author
There was a problem hiding this comment.
Thanks for your comments! I tried to add these checks but I found in most cases allocation->numRuns() == 1. Is it as expected, or did I misunderstand the above logic? Please review the updated code.
Contributor
There was a problem hiding this comment.
I think the number of page runs is most likely 1. I previous comment on the expected page run is incorrect. But you need to update allocation object as newRun() will insert a new run which update allocation_: AllocationPool::allocationAt(currentRunIndex()). Then it suppose all the newRun should have only one pageRun.
xiaoxmeng
reviewed
Apr 23, 2023
| const size_t kExceedLargestSizeClass = instance_->largestSizeClass() + 1; | ||
| AllocationPool pool(pool_.get()); | ||
| const auto* allocation = pool.allocationAt(0); | ||
| pool.newRun(1 * AllocationTraits::kPageSize); |
Contributor
There was a problem hiding this comment.
nit: AllocationTraits::pageBytes(numPages)
| } | ||
|
|
||
| TEST_P(MemoryAllocatorTest, exceedLargestSizeClass) { | ||
| const size_t kExceedLargestSizeClass = instance_->largestSizeClass() + 1; |
Contributor
There was a problem hiding this comment.
I think the number of page runs is most likely 1. I previous comment on the expected page run is incorrect. But you need to update allocation object as newRun() will insert a new run which update allocation_: AllocationPool::allocationAt(currentRunIndex()). Then it suppose all the newRun should have only one pageRun.
Contributor
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Collaborator
Author
|
@xiaoxmeng Can we merge this PR? |
Contributor
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
Collaborator
Author
|
@xiaoxmeng Here's the stacktrace. It's triggered when calling |
Collaborator
Author
|
@xiaoxmeng Do you have further comments? Can you provide the internal linter error for me to fix that? |
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 7, 2023
Summary: This function is used in Spark Runtime Filters: apache/spark#35789 https://docs.google.com/document/d/16IEuyLeQlubQkH8YuVuXWKo2-grVIoDJqQpHZrE7q04/edit#heading=h.4v65wq7vzy4q BloomFilter implementation in Velox is different from Spark, hence, serialized BloomFilter is different. Velox has memory limit for contiguous memory buffer, hence BloomFilter capacity is less than in Spark when numBits is large. See #4713 (comment) Spark allows for changing the defaults while Velox does not. See also #3342 Fixes #3694 Pull Request resolved: #4028 Reviewed By: Yuhta Differential Revision: D46352733 Pulled By: mbasmanova fbshipit-source-id: 1c8a0b489a736e627ba2c0869688fc0cf46279bb
Collaborator
Author
|
@xiaoxmeng Sorry for not catching up this PR for long time. Do you still have further comments? We still need this fix. |
|
@xiaoxmeng @mbasmanova Any issue to follow up on the PR? It's a bug fix of a Gluten case |
Contributor
|
@FelixYBW Would you please rebase? |
Collaborator
Author
|
Close this one since there are a lot of changes on main branch and we no longer need this PR. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
When directly invoking AllocationPoll::newRun like code piece below:
velox/velox/common/memory/HashStringAllocator.cpp
Line 154 in 35ab8cc
If the value of param
neededexceed largestSizeClass, the program fails with exception thrownThis is caused by passing the
numPagesas theminSizeClassand it fails the check herevelox/velox/common/memory/MemoryAllocator.cpp
Lines 51 to 56 in 35ab8cc