Skip to content

TaggedCache: Fix swept object memory release which can occur within the … #4242

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

Closed
wants to merge 3 commits into from
Closed

TaggedCache: Fix swept object memory release which can occur within the … #4242

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cddae94
Release TaggedCache object memory outside the lock
greg7mdp Jul 18, 2022
587db57
Remove unreferenced parameter name as suggested in code review
greg7mdp Jul 27, 2022
72f6e44
restore pattern of passing a lock reference to ensure a function is c…
greg7mdp Aug 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions src/ripple/basics/TaggedCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,14 +196,17 @@ class TaggedCache
return true;
}

using SweptPointersVector = std::pair<
std::vector<std::shared_ptr<mapped_type>>,
std::vector<std::weak_ptr<mapped_type>>>;

void
sweep()
{
// Keep references to all the stuff we sweep
// For performance, each worker thread should exit before the swept data
// is destroyed but still within the main cache lock.
std::vector<std::vector<std::shared_ptr<mapped_type>>> allStuffToSweep(
m_cache.partitions());
std::vector<SweptPointersVector> allStuffToSweep(m_cache.partitions());

clock_type::time_point const now(m_clock.now());
clock_type::time_point when_expire;
Expand Down Expand Up @@ -652,17 +655,18 @@ class TaggedCache
clock_type::time_point const& when_expire,
[[maybe_unused]] clock_type::time_point const& now,
typename KeyValueCacheType::map_type& partition,
std::vector<std::shared_ptr<mapped_type>>& stuffToSweep,
SweptPointersVector& stuffToSweep,
std::atomic<int>& allRemovals,
std::lock_guard<std::recursive_mutex> const& lock)
std::lock_guard<std::recursive_mutex> const&)
{
return std::thread([&, this]() {
int cacheRemovals = 0;
int mapRemovals = 0;

// Keep references to all the stuff we sweep
// so that we can destroy them outside the lock.
stuffToSweep.reserve(partition.size());
stuffToSweep.first.reserve(partition.size());
stuffToSweep.second.reserve(partition.size());
{
auto cit = partition.begin();
while (cit != partition.end())
Expand All @@ -672,6 +676,8 @@ class TaggedCache
// weak
if (cit->second.isExpired())
{
stuffToSweep.second.push_back(
std::move(cit->second.weak_ptr));
++mapRemovals;
cit = partition.erase(cit);
}
Expand All @@ -686,7 +692,8 @@ class TaggedCache
++cacheRemovals;
if (cit->second.ptr.use_count() == 1)
{
stuffToSweep.push_back(cit->second.ptr);
stuffToSweep.first.push_back(
std::move(cit->second.ptr));
++mapRemovals;
cit = partition.erase(cit);
}
Expand Down Expand Up @@ -722,17 +729,16 @@ class TaggedCache
clock_type::time_point const& when_expire,
clock_type::time_point const& now,
typename KeyOnlyCacheType::map_type& partition,
std::vector<std::shared_ptr<mapped_type>>& stuffToSweep,
SweptPointersVector&,
std::atomic<int>& allRemovals,
std::lock_guard<std::recursive_mutex> const& lock)
std::lock_guard<std::recursive_mutex> const&)
{
return std::thread([&, this]() {
int cacheRemovals = 0;
int mapRemovals = 0;

// Keep references to all the stuff we sweep
// so that we can destroy them outside the lock.
stuffToSweep.reserve(partition.size());
{
auto cit = partition.begin();
while (cit != partition.end())
Expand Down