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

Limit the cluster cache by memory instead of number of clusters #956

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+645 −209
Open

Limit the cluster cache by memory instead of number of clusters #956

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7ab76f9
Make ConcurentCache inherit lru_cache.
mgautierfr Mar 3, 2025
a21ee25
Make caches open to a cost of each item different than 1
mgautierfr Feb 7, 2025
71a496f
Add `getMemorySize` on `IStreamReader` and `Reader`.
mgautierfr Feb 11, 2025
1add1d3
Limit the cluster cache by memory instead of numbers of items.
mgautierfr Feb 11, 2025
dc82b88
Drop all zim clusters from cache at zim file destruction.
mgautierfr Feb 14, 2025
a5032d2
Test archive against a preloaded content.
mgautierfr Feb 17, 2025
61c863e
Make the cluster cache global.
mgautierfr Feb 14, 2025
8c91f85
Reset global cache size before each archive test.
mgautierfr Feb 14, 2025
9b9e229
Ensure cluster cache is properly clean in case of exception in constr…
mgautierfr Feb 14, 2025
f896c0f
Check if shared_future has been drop while we compute the value.
mgautierfr Mar 5, 2025
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
15 changes: 12 additions & 3 deletions src/concurrent_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
return 0;
}
}

Check notice on line 55 in src/concurrent_cache.h

View check run for this annotation

codefactor.io / CodeFactor

src/concurrent_cache.h#L55 

Redundant blank line at the end of a code block should be deleted. (whitespace/blank_line)
};

/**
Expand Down Expand Up @@ -90,23 +90,32 @@
{
std::promise<Value> valuePromise;
std::unique_lock<std::mutex> l(lock_);
const auto x = Impl::getOrPut(key, valuePromise.get_future().share());
auto shared_future = valuePromise.get_future().share();
const auto x = Impl::getOrPut(key, shared_future);
l.unlock();
if ( x.miss() ) {
try {
valuePromise.set_value(f());
auto cost = CostEstimation::cost(x.value().get());
// There is a small window when the valuePromise may be drop from lru cache after
// we set the value but before we increase the size of the cache.
// In this case decrease the size of `cost` before increasing it.
// In this case we decrease the size of `cost` before increasing it.
// First of all it should be pretty rare as we have just put the future in the cache so it
// should not be the least used item.
// If it happens, this should not be a problem if current_size is bigger than `cost` (most of the time)
// For the really rare specific case of current cach size being lower than `cost` (if possible),
// `decreaseCost` will clamp the new size to 0.
{
std::unique_lock<std::mutex> l(lock_);
Impl::increaseCost(cost);
// There is a window when the shared_future is drop from the cache while we are computing the value.
// If this is the case, we readd the shared_future in the cache.
if (!Impl::exists(key)) {
// We don't have have to increase the cache as the future is already set, so the cost will be valid.
Impl::put(key, shared_future);
} else {
// We just have to increase the cost as we used 0 for unset future.
Impl::increaseCost(cost);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

While working on #960 I figured out that this doesn't fix the problem described in #956 (comment) & #956 (comment) (or replaces it with another one with a similar effect).

Consider that "the shared_future is drop from the cache while we are computing the value.". Additionally, while that entry is missing from the cache, a request for the same key is concurrently made, resulting in a second concurrent process of adding that same entry to the cache. The latter quickly sets up the placeholder entry and proceeds to computing the value. In the meantime, the first process of computing the value completes, finds out that the key is present in the cache and increases the cost value. After a while, its sibling process unknowingly does the same. So apart from double work (and temporary double memory usage of by two instances of the same value only one of which ends up in the cache) we are left with double-counted cost of the slow item.

The issue with double-counting is solved in my redesign of ConcurrentCache. The double-work issue is demonstrated in 551fe87 and addressed in e34bd71 .

} catch (std::exception& e) {
drop(key);
Expand Down
Loading