Skip to content

Commit

Permalink
Bug 1875859 - Refactor MemoryPool::PurgeExpired and PurgeByFrecency t…
Browse files Browse the repository at this point in the history
…o reduce overhead and protect against races while sorting. r=necko-reviewers,valentin

A `CacheEntry`'s `mExpirationTime` and `mFrecency` can be updated asynchronously while sorting. This is at best undefined behavior and at worst it can derail `std::sort`'s algorithm out of bounds.
Looking a bit closer it turns out that sorting for `PurgeExpired` is actually not needed and we can have just a `mManagedEntries` linked list in a memory pool instead of two arrays. This way we can also achieve constant O(1) time for unregistering entries when purging.
`PurgeByFrecency` needs sorting and must ensure to not race with modifications of `mFrecency`. We ensure to make some progress on each run to compensate for the cost of sorting.
We also do `PurgeExpired` always, not only if under memory pressure, as it is cheap (no sorting) and interuptable., as well as an emergency handling if `PurgeByFrecency` failed to allocate the auxiliary sort array.

Differential Revision: https://phabricator.services.mozilla.com/D199955
  • Loading branch information
jensstutte committed Feb 12, 2024
1 parent 4cc9a70 commit 5283906
Show file tree
Hide file tree
Showing 4 changed files with 179 additions and 143 deletions.
18 changes: 18 additions & 0 deletions modules/libpref/init/StaticPrefList.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12695,6 +12695,24 @@
value: false
mirror: always

# CacheStorageService::MemoryPool::PurgeByFrecency does a rather expensive
# sort we cannot interrupt by an incoming cache operation, so we want the
# purging to do some progress to not have just wasted cycles.
# Initial default 0, see bug 1879814.
- name: network.cache.purgebyfrecency_minprogress_disk
type: RelaxedAtomicUint32
value: 0
mirror: always

# CacheStorageService::MemoryPool::PurgeByFrecency does a rather expensive
# sort we cannot interrupt by an incoming cache operation, so we want the
# purging to do at least some progress to not have just wasted cycles.
# Initial default 0, see bug 1879814.
- name: network.cache.purgebyfrecency_minprogress_memory
type: RelaxedAtomicUint32
value: 0
mirror: always

# When true we will dispatch a background task (separate process) to
# delete the cache folder at shutdown in order to avoid shutdown hangs.
- name: network.cache.shutdown_purge_in_background_task
Expand Down
6 changes: 5 additions & 1 deletion netwerk/cache2/CacheEntry.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef CacheEntry__h__
#define CacheEntry__h__

#include "mozilla/LinkedList.h"
#include "nsICacheEntry.h"
#include "CacheFile.h"

Expand Down Expand Up @@ -42,7 +43,10 @@ class CacheStorage;
class CacheOutputCloseListener;
class CacheEntryHandle;

class CacheEntry final : public nsIRunnable, public CacheFileListener {
class CacheEntry final : public nsIRunnable,
public CacheFileListener,
// Used by CacheStorageService::MemoryPool
public LinkedListElement<RefPtr<CacheEntry>> {
public:
NS_DECL_THREADSAFE_ISUPPORTS
NS_DECL_NSIRUNNABLE
Expand Down
Loading

0 comments on commit 5283906

Please sign in to comment.