Remove inheritence relationship between AsyncDataCache and MemoryAllo…#5503
Closed
tanjialiang wants to merge 1 commit intofacebookincubator:mainfrom
Closed
Remove inheritence relationship between AsyncDataCache and MemoryAllo…#5503tanjialiang wants to merge 1 commit intofacebookincubator:mainfrom
tanjialiang wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
xiaoxmeng
reviewed
Jul 5, 2023
xiaoxmeng
reviewed
Jul 5, 2023
Contributor
xiaoxmeng
left a comment
xiaoxmeng
left a comment
There was a problem hiding this comment.
@tanjialiang some comments. Thanks!
|
|
||
| protected: | ||
| MemoryAllocator() = default; | ||
| explicit MemoryAllocator() = default; |
| MemoryAllocator() = default; | ||
| explicit MemoryAllocator() = default; | ||
|
|
||
| virtual bool allocateContiguousImpl( |
Contributor
There was a problem hiding this comment.
nit: not sure if allocateContiguousWithoutRetry is better?
| /// allocation fails. 'reservationCB' is used in the same way as allocate | ||
| /// does. It may throw and the end state will be consistent, with no new | ||
| /// allocation and 'allocation' and 'collateral' cleared. | ||
| /// allocation and 'allocation' and 'collateral' cleared. The allocator will |
Contributor
There was a problem hiding this comment.
The function might retry allocation failure by making space from 'cache_' if set.
dittos
| /// A structure that uses 'allocator_' to consume memory, and is also able to | ||
| /// free up memory upon request by shrinking itself, to give memory back to | ||
| /// 'allocator_'. | ||
| class Shrinkable { |
Contributor
There was a problem hiding this comment.
class MemoryAllocator::Cache {
virtual bool makeSpace(memory::MachinePageCount numPages, std::function<bool()> allocate) = 0;
| /// the kind of the delegated memory allocator underneath. | ||
| virtual Kind kind() const = 0; | ||
|
|
||
| void setShrinkable(const std::shared_ptr<Shrinkable>& shrinkable) { |
Contributor
There was a problem hiding this comment.
s/setShrinkable/setCache/ and add to check we only allow to set it once from null to non-null? thanks!
tanjialiang
force-pushed
the
sep_cache
branch
7 times, most recently
from
171fea8 to
bd736d8
Compare
xiaoxmeng
reviewed
Jul 13, 2023
Contributor
xiaoxmeng
left a comment
xiaoxmeng
left a comment
There was a problem hiding this comment.
@tanjialiang Looks great. Thanks for interating!
| public: | ||
| /// This method should be implemented so that it tries to accommodate the | ||
| /// passed in 'allocate' by freeing up space from 'this' if needed. 'numPages' | ||
| /// is the number of pages 'allocate' tries to allocate. The default |
Contributor
There was a problem hiding this comment.
The function should return true if 'allocate' succeeds, and false otherwise.
Remove dummy part.
| /// Unregisters the 'Cache' that was previously registered. It is the caller's | ||
| /// responsibility to unregister the cache upon destruction of the 'Cache'. | ||
| /// | ||
| /// NOTE: Caller should make sure that unregistration of 'Cache' shall happen |
Contributor
There was a problem hiding this comment.
NOTE: the caller should ensures that unregistration of 'Cache' shall happen before its destruction and all the allocated cached memory has been freed.
It might not be safe to unregister the cache as the memory allocator can access the cache at any point so there is a race condition there. I don't think we really need unregister cache operation. Let's hold a shared reference on the cache from the allocator. And make sure there is no memory usage from cache when memory allocator destroys. We could add the allocated memory in cache interface.
| Allocation& out, | ||
| ReservationCallback reservationCB, | ||
| MachinePageCount minSizeClass) { | ||
| if (!cache_) { |
Contributor
There was a problem hiding this comment.
nit: if (cache_ == nullptr)
| ContiguousAllocation& allocation, | ||
| ReservationCallback reservationCB); | ||
|
|
||
| void freeContiguous(ContiguousAllocation& allocation) override; |
Contributor
There was a problem hiding this comment.
This is a public method?
| } | ||
|
|
||
| AsyncDataCache::~AsyncDataCache() { | ||
| allocator_->unregisterCache(this); |
Contributor
There was a problem hiding this comment.
Have a check that all memory have been freed instead.
tanjialiang
force-pushed
the
sep_cache
branch
3 times, most recently
from
7884ac8 to
4df1851
Compare
xiaoxmeng
reviewed
Jul 15, 2023
Contributor
xiaoxmeng
left a comment
xiaoxmeng
left a comment
There was a problem hiding this comment.
@tanjialiang thanks for the update and iterations!
| allocator_ = std::make_shared<cache::AsyncDataCache>( | ||
| allocator, memoryBytes, std::move(ssdCache)); | ||
| allocator_ = std::make_shared<memory::MmapAllocator>(options); | ||
| cache_ = cache::AsyncDataCache::createAsyncDataCache( |
Contributor
There was a problem hiding this comment.
s/cache::AsyncDataCache::createAsyncDataCache/cache::AsyncDataCache::create/
|
|
||
| AsyncDataCache::AsyncDataCache( | ||
| const std::shared_ptr<MemoryAllocator>& allocator, | ||
| memory::MemoryAllocator* const allocator, |
Contributor
There was a problem hiding this comment.
Drop pointer const qualifier as we pass the pointer by value.
|
|
||
| AsyncDataCache::AsyncDataCache( | ||
| const std::shared_ptr<MemoryAllocator>& allocator, | ||
| memory::MemoryAllocator* const allocator, |
| public: | ||
| // TODO(jtan6): Remove this constructor after Presto Native switches to below | ||
| // constructor | ||
| AsyncDataCache( |
Contributor
There was a problem hiding this comment.
We can remove this ctor now?
|
|
||
| // static | ||
| std::shared_ptr<AsyncDataCache> AsyncDataCache::createAsyncDataCache( | ||
| memory::MemoryAllocator* const allocator, |
Contributor
There was a problem hiding this comment.
ditto
AsyncDataCache::create
| } | ||
|
|
||
| // static | ||
| AsyncDataCache*& AsyncDataCache::getInstanceInternal() { |
Contributor
There was a problem hiding this comment.
AsyncDataCache** AsyncDataCache::getInstancePtr() {
return &cache_;
}
AsyncDataCache* AsyncDataCache::getInstancePtr() {
return *getInstanceInternal();
}
velox/common/memory/MmapAllocator.h
Outdated
| std::unique_ptr<ManagedMmapArenas> managedArenas_; | ||
|
|
||
| std::shared_ptr<Cache> cache_; | ||
| Stats stats_; |
Contributor
There was a problem hiding this comment.
Do we still stats_ here?
tanjialiang
force-pushed
the
sep_cache
branch
2 times, most recently
from
ae2e0d3 to
7a7f9ad
Compare
Contributor
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
xiaoxmeng
approved these changes
Jul 18, 2023
tanjialiang
force-pushed
the
sep_cache
branch
5 times, most recently
from
aa7316f to
db4a5e4
Compare
Contributor
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
tanjialiang
added a commit
to tanjialiang/velox-1
that referenced
this pull request
Jul 21, 2023
facebookincubator#5503) Summary: Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change. Pull Request resolved: facebookincubator#5503 Reviewed By: xiaoxmeng Differential Revision: D47536273 Pulled By: tanjialiang fbshipit-source-id: ce91a4eaf46297a5146ea90743b0e8f0833ab13d
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47536273 |
tanjialiang
added a commit
to tanjialiang/velox-1
that referenced
this pull request
Jul 21, 2023
facebookincubator#5503) Summary: Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change. Pull Request resolved: facebookincubator#5503 Reviewed By: xiaoxmeng Differential Revision: D47536273 Pulled By: tanjialiang fbshipit-source-id: e1edc0d967b211fc092adfeac02aefc3276a7a9c
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47536273 |
facebookincubator#5503) Summary: X-link: prestodb/presto#20372 Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change. Pull Request resolved: facebookincubator#5503 Reviewed By: xiaoxmeng Differential Revision: D47536273 Pulled By: tanjialiang fbshipit-source-id: 9a02e8dc5e1f05a00d4cd7e2828fdd5e8648669e
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47536273 |
xiaoxmeng
reviewed
Jul 25, 2023
| } | ||
| } | ||
|
|
||
| void CacheShard::prepareShutdown() { |
Contributor
There was a problem hiding this comment.
just name it as shutdown()
|
|
||
| static void setInstance(AsyncDataCache* asyncDataCache); | ||
|
|
||
| /// Release any resources that consume memory from 'allocator_' for a graceful |
Contributor
There was a problem hiding this comment.
/// Release any memory allocated from 'allocator_' for a graceful shutdown.
///
/// NOTE: user can not access cache after this call as the cache is no longer valid.
Contributor
|
@tanjialiang merged this pull request in ad9ffa1. |
tanjialiang
added a commit
to tanjialiang/presto
that referenced
this pull request
Jul 25, 2023
prestodb#20372) Summary: Pull Request resolved: prestodb#20372 Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change. X-link: facebookincubator/velox#5503 Reviewed By: xiaoxmeng Differential Revision: D47536273 Pulled By: tanjialiang fbshipit-source-id: 7749b787bf2a1027b33e02690917c59bb497026c
mbasmanova
pushed a commit
to prestodb/presto
that referenced
this pull request
Jul 26, 2023
#20372) Summary: Pull Request resolved: #20372 Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change. X-link: facebookincubator/velox#5503 Reviewed By: xiaoxmeng Differential Revision: D47536273 Pulled By: tanjialiang fbshipit-source-id: 7749b787bf2a1027b33e02690917c59bb497026c
wypb
pushed a commit
to wypb/presto
that referenced
this pull request
Dec 22, 2023
prestodb#20372) Summary: Pull Request resolved: prestodb#20372 Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change. X-link: facebookincubator/velox#5503 Reviewed By: xiaoxmeng Differential Revision: D47536273 Pulled By: tanjialiang fbshipit-source-id: 7749b787bf2a1027b33e02690917c59bb497026c
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
3 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.
Removing the relationship of AsyncDataCache inheritance from MemoryAllocator. Now they are depending on each other with registration mechanism. Related tests are refactored to be consistent with the new change.