Feature/instrumented mutex wrapper #20726

maierlars · 2024-03-12T09:15:49Z

Scope & Purpose

This PR adds an InstrumentedMutex wrapper class that collects metrics about the mutex it instruments. These metrics contain:

number of pending shared/exclusive lock requests
number of shared/exclusive locks.

Later I want to add Histograms tracking the lock and wait time. But this PR is already big enough.

As an application the exclusive lock in the RocksDBMetaCollection has been instrumented and new metrics were introduced. As a side effect, the locking methods now return a guard object that is stored in the RocksDBTransactionCollection object.

This guard object is necessary for reconstructing the lock times. Otherwise, one can not assign the lock and unlock of a shared lock.

…-mutex-wrapper

jsteemann · 2024-04-10T17:05:41Z

Documentation/Metrics/arangodb_vocbase_meta_collection_lock_locked_exclusive.yaml

+  - agent
+  - single
+description: |
+  Counts the exclusively locked collection across all shards in a database.


Suggested change

Counts the exclusively locked collection across all shards in a database.

Counts the exclusively locked collections across all shards in a database.

Maybe also note that on a DB server, the number reported by the metric is the number of exclusively locked shards, not collections.

jsteemann · 2024-04-10T17:07:15Z

Documentation/Metrics/arangodb_vocbase_meta_collection_lock_locked_exclusive.yaml

@@ -0,0 +1,14 @@
+name: arangodb_vocbase_meta_collection_lock_locked_exclusive


I don't think we should name a publicly visible metric something like "vocbase". "vocbase" is some internal term that doesn't make much sense to end users. The same is true for "meta_collection".
Why not go with some simpler name such as "arangodb_collection_locks_locked_*"? If that name is already in use, maybe some similar easy alternative?

jsteemann · 2024-04-10T17:08:22Z

Documentation/Metrics/arangodb_vocbase_meta_collection_lock_locked_shared.yaml

+  - agent
+  - single
+description: |
+  Counts the shared locked collection across all shards in a database.


Suggested change

Counts the shared locked collection across all shards in a database.

Counts the shared locked collections across all shards in a database.

jsteemann · 2024-04-10T17:09:44Z

arangod/Metrics/CMakeLists.txt

@@ -14,7 +14,8 @@ add_library(arango_metrics STATIC
  MetricsFeature.cpp
  ClusterMetricsFeature.cpp
  ${PROJECT_SOURCE_DIR}/arangod/RestHandler/RestMetricsHandler.cpp
-  ${PROJECT_SOURCE_DIR}/arangod/RestHandler/RestUsageMetricsHandler.cpp)
+  ${PROJECT_SOURCE_DIR}/arangod/RestHandler/RestUsageMetricsHandler.cpp
+        InstrumentedMutex.cpp)


nit: the indentation here is somewhat unexpected, but it doesn't matter much.

jsteemann · 2024-04-10T17:12:44Z

arangod/Metrics/InstrumentedMutex.h

+
+  template<typename F, typename Duration>
+  auto try_lock_shared_for(Mutex& m, Duration d, F&& fn) requires requires {
+    m.try_lock_for(d);


Suggested change

m.try_lock_for(d);

m.try_lock_shared_for(d);

jsteemann · 2024-04-10T17:26:23Z

arangod/RocksDBEngine/RocksDBReplicationContext.cpp

-    } else {
-      lockGuard.cancel();
+    if (!_patchCount.empty() && _patchCount == cname) {
+      auto [guard, res] = co_await rcoll->lockExclusive(to);


I think this leads to changed behavior.
In the old implementation, the lock acquired by rcoll->lockWrite(to) acquired on line old:269 could be held until the end of the entire code block. The lock may have been released only on line old:289.

In the new implementation, we are acquiring the lock in line 268 and release it on line 273 already, which is way earlier than before.
So with the new implementation, even if the lock acquistion is successful, we don't execute the following code under the lock, but previously we did:

// re-acquire the mutex. we need it for checking and modifying _iterators. writeLocker.lock(); it = _iterators.find(cid); if (it != _iterators.end()) { // someone else inserted the iterator in-between co_return std::make_tuple(Result{}, it->second->logical->id(), it->second->numberDocuments); } numberDocuments = rcoll->meta().numberDocuments(); lazyCreateSnapshot();

jsteemann · 2024-04-10T17:28:01Z

arangod/RocksDBEngine/RocksDBTransactionCollection.h

@@ -177,5 +178,8 @@ class RocksDBTransactionCollection : public TransactionCollection {

  bool _usageLocked;
  bool _exclusiveWrites;
+  std::variant<std::monostate, RocksDBMetaCollection::ExclusiveLock,


nit: as we are now using std::variant in this header file, we should also add #include <variant> at the top of the file.

jsteemann · 2024-04-10T17:29:18Z

lib/Basics/FutureSharedLock.h

+
+  void unlock_shared(LockGuard&& guard) { guard.unlock(); }
+
+  bool owns_lock(LockGuard& guard) { return guard.isLocked(); }


nit:

Suggested change

bool owns_lock(LockGuard& guard) { return guard.isLocked(); }

bool owns_lock(LockGuard const& guard) const noexcept { return guard.isLocked(); }

jsteemann · 2024-04-10T17:31:13Z

tests/Metrics/InstrumentedMutexTest.cpp

+  InstrumentedMutex<std::shared_mutex> m{metrics};
+
+  ASSERT_EQ(lockExclusive.load(), 0);
+  ASSERT_EQ(pendingExclusive.load(), 0);


we could check here that lockShared is 0.

jsteemann · 2024-04-10T17:31:21Z

tests/Metrics/InstrumentedMutexTest.cpp

+  ASSERT_TRUE(guard);
+
+  ASSERT_EQ(lockExclusive.load(), 1);
+  ASSERT_EQ(pendingExclusive.load(), 0);


we could check here that lockShared is still 0.

jsteemann · 2024-04-10T17:31:37Z

tests/Metrics/InstrumentedMutexTest.cpp

+  guard.unlock();
+  ASSERT_FALSE(guard.owns_lock());
+  ASSERT_EQ(lockExclusive.load(), 0);
+  ASSERT_EQ(pendingExclusive.load(), 0);


jsteemann · 2024-04-10T17:32:34Z

...s/client/shell/transaction/shell-transaction-rocksdb-meta-collection-lock-metrics-cluster.js

+      db._createDatabase(database);
+      db._useDatabase(database);
+      db._create(collection, {numberOfShards: 1, replicationFactor: 1});
+      waitForShardsInSync(collection);


nit: with numberOfShards == 1 && replicationFactor == 1, I don't think we need to wait for any shard to get into sync.

jsteemann · 2024-04-10T17:35:45Z

...s/client/shell/transaction/shell-transaction-rocksdb-meta-collection-lock-metrics-cluster.js

+
+    testSharedLock: function () {
+      {
+        assertMetrics({


I think this entire test can spuriously fail if there is a write operation ongoing in some other collection in the background. During the tests, this can possibly happen due to some statistics background thread writing data into any of the statistics collections or running a cleanup query on them.

What we could do to work around interferences by temporary background operations is to add a few retries to assertMetrics, until the desired target state has been reached or some timeout has been exceeded.

jsteemann · 2024-04-10T17:36:04Z

...s/client/shell/transaction/shell-transaction-rocksdb-meta-collection-lock-metrics-cluster.js

+
+    testExclusiveLock: function () {
+      {
+        assertMetrics({


same issue here as mentioned for above test.

jsteemann · 2024-04-10T17:38:12Z

arangod/RocksDBEngine/RocksDBEngine.cpp

@@ -2058,11 +2058,6 @@ Result RocksDBEngine::dropCollection(TRI_vocbase_t& vocbase,
  bool const prefixSameAsStart = true;
  bool const useRangeDelete = rcoll->meta().numberDocuments() >= 32 * 1024;

-  auto resLock = rcoll->lockWrite().get();  // technically not necessary


While the comments mentions that this is "technically not necessary", acquiring the lock here may previously have led to synchronization with all ongoing write operations to the collection being fully finished.
With the lock being removed here now, that behavior may or may not change. I can't tell.
To be on the safe side, we could add the lock back even if we can't prove it is actually needed. Or we try and see.

maierlars added 14 commits March 7, 2024 16:38

First version of instrumented mutex.

58ea91f

Adding more cases.

5d34ae7

Fix compilation.

965af71

Added InstrumentedMutex to Meta Collection.

d586017

Fixing lock usage.

252df55

Add explicit template for lock in RocksDBMetaCollection.

55478d5

Merge remote-tracking branch 'origin/devel' into feature/instrumented…

f2c75ed

…-mutex-wrapper

Merge remote-tracking branch 'origin/devel' into feature/instrumented…

07fe603

…-mutex-wrapper

Add minimal tests for InstrumentedMutex.

1c35021

Added metrics for the RocksDBMetaCollectionLock.

fe68ff9

Add js test for metrics.

8eccda1

Fix test.

11ede9a

Renamed to lockShared/lockExclusive.

f2416f2

Add metrics descriptions.

0ef12e6

maierlars self-assigned this Mar 12, 2024

cla-bot bot added the cla-signed label Mar 12, 2024

maierlars marked this pull request as ready for review March 13, 2024 10:08

maierlars requested review from a team as code owners March 13, 2024 10:08

maierlars added 2 commits March 22, 2024 09:23

Merge remote-tracking branch 'origin/devel' into feature/instrumented…

3d0db13

…-mutex-wrapper

Merge remote-tracking branch 'origin/devel' into feature/instrumented…

31fd99f

…-mutex-wrapper

maierlars force-pushed the feature/instrumented-mutex-wrapper branch from 9f5b86b to 31fd99f Compare April 5, 2024 13:49

Merge remote-tracking branch 'origin/devel' into feature/instrumented…

b933596

…-mutex-wrapper

jsteemann added this to the devel milestone Apr 10, 2024

jsteemann reviewed Apr 10, 2024

View reviewed changes

maierlars added 3 commits April 11, 2024 10:40

Adressed some review comments.

4b8ca20

Renamed metrics.

8c387e1

Clarify that this is about shards, not collections.

9244ea6

Simran-B mentioned this pull request May 15, 2024

DOC-667 | Cluster metrics about read-only shards and collection shard locks arangodb/docs-hugo#532

Open

maierlars closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/instrumented mutex wrapper #20726

Feature/instrumented mutex wrapper #20726

maierlars commented Mar 12, 2024 •

edited

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

jsteemann Apr 10, 2024

	Counts the exclusively locked collection across all shards in a database.
	Counts the exclusively locked collections across all shards in a database.

		@@ -0,0 +1,14 @@
		name: arangodb_vocbase_meta_collection_lock_locked_exclusive

	Counts the shared locked collection across all shards in a database.
	Counts the shared locked collections across all shards in a database.


		void unlock_shared(LockGuard&& guard) { guard.unlock(); }

		bool owns_lock(LockGuard& guard) { return guard.isLocked(); }

	bool owns_lock(LockGuard& guard) { return guard.isLocked(); }
	bool owns_lock(LockGuard const& guard) const noexcept { return guard.isLocked(); }

Feature/instrumented mutex wrapper #20726

Feature/instrumented mutex wrapper #20726

Conversation

maierlars commented Mar 12, 2024 • edited

Scope & Purpose

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maierlars commented Mar 12, 2024 •

edited