Improve memory efficiency of many OptimisticTransactionDBs #11439
Conversation
Summary: Currently it's easy to use a ton of memory with many small OptimisticTransactionDB instances, because each one by default allocates (by default) a million mutexes (40 bytes each on my compiler) for validating transactions. It even puts a lot of pressure on the allocator by allocating each one individually! In this change: * Create a new object and option that enables sharing these buckets of mutexes between instances. This is generally good for load balancing potential contention as various DBs become hotter or colder with txn writes. About the only cases where this sharing wouldn't make sense (e.g. each DB usually written by one thread) are cases that would be better off with OccValidationPolicy::kValidateSerial which doesn't use the buckets anyway. * Allocate the mutexes in a contiguous array, for efficiency * Add an option to ensure the mutexes are cache-aligned. In several other places we use cache-aligned mutexes but OptimisticTransactionDB historically does not. It should be a space-time trade-off the user can choose. * Provide some visibility into the memory used by the mutex buckets with an ApproximateMemoryUsage() function (also used in unit testing) * Share code with other users of "striped" mutexes, appropriate refactoring for customization & efficiency (e.g. using FastRange instead of modulus) Test Plan: unit tests added. Ran sized-up versions of stress test in unit test, including a before-and-after performance test showing no consistent difference. (NOTE: OptimisticTransactionDB not currently covered by db_stress!)
pdillinger
force-pushed
the
optimistic_txn_memory
branch
from
2317ef8
to
d2dc631
Compare
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| // Most details in internal derived class | ||
| // Users should not derive from this class |
There was a problem hiding this comment.
We could consider enforcing this (instead of relying on users) using the pimpl idiom:
class OccLockBuckets {
public:
~OccLockBuckets(); // non-virtual, implemented in .cc
size_t ApproximateMemoryUsage() const; // non-virtual, gets forwarded to impl_->ApproximateMemoryUsage(), implemented in .cc
private:
// private ctor, implemented in .cc
// friends as needed (e.g. factory method)
class ImplBase;
std::unique_ptr<ImplBase> impl_;
};
There was a problem hiding this comment.
I don't like the additional level of indirection, and wanted to avoid some boilerplate, but I'll include the version without indirection.
| for (auto v : lk_ptrs) { | ||
| v->Lock(); | ||
| } | ||
| Defer unlocks([&]() { | ||
| for (auto v : lk_ptrs) { | ||
| v->Unlock(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I realize that (at least per the style guide) exceptions don't exist ;) but this is slightly less exception safe than the original. Building a vector of unique_locks as we iterate means that if an exception is thrown during a call to Lock, keys locked earlier get automatically unlocked; here, we only ensure that if every mutex was successfully locked, each will also get unlocked.
There was a problem hiding this comment.
I'll note this in a comment.
| template <bool cache_aligned> | ||
| class OccLockBucketsImpl : public OccLockBucketsImplBase { | ||
| public: | ||
| OccLockBucketsImpl(size_t bucket_count) : locks_(bucket_count) {} |
There was a problem hiding this comment.
Might want to mark this ctor explicit or the linter might complain
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
pdillinger
force-pushed
the
optimistic_txn_memory
branch
from
60eed0d
to
db40f19
Compare
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger merged this pull request in 17bc277. |
Summary: Currently it's easy to use a ton of memory with many small OptimisticTransactionDB instances, because each one by default allocates a million mutexes (40 bytes each on my compiler) for validating transactions. It even puts a lot of pressure on the allocator by allocating each one individually!
In this change:
Test Plan: unit tests added. Ran sized-up versions of stress test in unit test, including a before-and-after performance test showing no consistent difference. (NOTE: OptimisticTransactionDB not currently covered by db_stress!)