mirror of
https://github.com/XRPLF/rippled.git
synced 2026-07-29 10:00:30 +00:00
fix(nodestore): sample writer depth over one population
Depth was observed at insert entry but its sample was only counted at insert exit, so an insert still in flight contributed nothing to the mean while completed inserts -- disproportionately the fast, shallow ones -- all did. The reported mean understated queueing exactly when queueing was worst: with every writer inside its first insert the gauge was omitted entirely, while writers-in-flight correctly showed them all. Depth and its sample count are now both folded in at entry, so the mean is taken over one population. Mean depth is the L in Little's Law, so a biased L understated the derived queueing share of each insert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -25,7 +25,7 @@ namespace xrpl::node_store {
|
||||
* |
|
||||
* concurrentWriters = queue length here
|
||||
* insertTotalUs / insertCount = time in the whole system (W)
|
||||
* depthSum / insertCount = mean depth (L)
|
||||
* depthSum / depthSamples = mean depth (L)
|
||||
*
|
||||
* @note All fields except @ref concurrentWriters are cumulative for the
|
||||
* life of the backend, so a reader must difference successive
|
||||
@@ -38,7 +38,7 @@ namespace xrpl::node_store {
|
||||
* if (auto const s = backend->getWriteStats(); s && s->insertCount)
|
||||
* {
|
||||
* double const meanUs = double(s->insertTotalUs) / s->insertCount;
|
||||
* double const meanDepth = double(s->depthSum) / s->insertCount;
|
||||
* double const meanDepth = double(s->depthSum) / s->depthSamples;
|
||||
* double const serviceUs = meanUs / meanDepth; // Little's Law
|
||||
* }
|
||||
* @endcode
|
||||
@@ -76,6 +76,15 @@ struct WriteStats
|
||||
* @ref insertCount this gives mean depth.
|
||||
*/
|
||||
std::uint64_t depthSum = 0;
|
||||
|
||||
/**
|
||||
* Number of depth samples summed into @ref depthSum.
|
||||
*
|
||||
* Divide @ref depthSum by this, not by @ref insertCount: a sample is
|
||||
* taken when an insert starts, while insertCount only rises when one
|
||||
* finishes, so the two populations differ while inserts are in flight.
|
||||
*/
|
||||
std::uint64_t depthSamples = 0;
|
||||
};
|
||||
|
||||
} // namespace xrpl::node_store
|
||||
|
||||
@@ -87,10 +87,21 @@ public:
|
||||
std::atomic<std::uint64_t> insertMaxUs{0};
|
||||
|
||||
/**
|
||||
* Summed depth observed at each insert.
|
||||
* Summed depth observed at each insert, accumulated at insert entry.
|
||||
*/
|
||||
std::atomic<std::uint64_t> depthSum{0};
|
||||
|
||||
/**
|
||||
* How many depth samples make up @ref depthSum.
|
||||
*
|
||||
* Its own counter rather than reusing the completed-insert count, because
|
||||
* the two are taken at different moments: a depth sample exists as soon
|
||||
* as an insert starts, while the insert count only rises when one
|
||||
* finishes. Dividing by the wrong one biases the mean downward under
|
||||
* load, which is when the mean matters.
|
||||
*/
|
||||
std::atomic<std::uint64_t> depthSamples{0};
|
||||
|
||||
NuDBBackend(
|
||||
size_t keyBytes,
|
||||
Section const& keyValues,
|
||||
@@ -270,7 +281,16 @@ public:
|
||||
// NuDB takes one global mutex for the whole insert, so the wait is
|
||||
// invisible from here. Record the depth we joined at and the wall
|
||||
// time we spent; the split follows from Little's Law.
|
||||
//
|
||||
// Depth and its sample count are both folded in HERE, at entry, so
|
||||
// the mean is over the same population. Counting the sample at exit
|
||||
// instead would drop every insert still in flight, and those are the
|
||||
// slow, deep ones -- biasing the mean down exactly when queueing is
|
||||
// worst. With all writers inside their first insert the exit-counted
|
||||
// version reports no depth at all.
|
||||
auto const depth = concurrentWriters.fetch_add(1, std::memory_order_relaxed) + 1;
|
||||
depthSum.fetch_add(depth, std::memory_order_relaxed);
|
||||
depthSamples.fetch_add(1, std::memory_order_relaxed);
|
||||
auto const begin = std::chrono::steady_clock::now();
|
||||
|
||||
// A scope guard rather than straight-line code, because the insert
|
||||
@@ -369,6 +389,7 @@ public:
|
||||
stats.insertTotalUs = insertTotalUs.load(std::memory_order_relaxed);
|
||||
stats.insertMaxUs = insertMaxUs.load(std::memory_order_relaxed);
|
||||
stats.depthSum = depthSum.load(std::memory_order_relaxed);
|
||||
stats.depthSamples = depthSamples.load(std::memory_order_relaxed);
|
||||
return stats;
|
||||
}
|
||||
|
||||
@@ -424,7 +445,6 @@ private:
|
||||
concurrentWriters.fetch_sub(1, std::memory_order_relaxed);
|
||||
insertCount.fetch_add(1, std::memory_order_relaxed);
|
||||
insertTotalUs.fetch_add(elapsedUs, std::memory_order_relaxed);
|
||||
depthSum.fetch_add(depth, std::memory_order_relaxed);
|
||||
|
||||
// std::atomic has no fetch_max, so raise the maximum with a CAS
|
||||
// loop. Mirrors the clamp loop in OTelCollector.cpp.
|
||||
|
||||
@@ -849,7 +849,7 @@ MetricsRegistry::observeWritePathDetail(node_store::Database const& db, ObserveF
|
||||
// above 1.0 even under load. An integral gauge would truncate that to 1
|
||||
// and lose the whole signal, hence the fixed-point scale -- which the
|
||||
// name states, so nobody reads 140 as 140 writers.
|
||||
if (auto const mean = scaledMean(ws->depthSum, ws->insertCount, 100))
|
||||
if (auto const mean = scaledMean(ws->depthSum, ws->depthSamples, 100))
|
||||
observe("nudb_writer_depth_x100", *mean);
|
||||
}
|
||||
|
||||
@@ -862,6 +862,13 @@ MetricsRegistry::observeAcquireStats(AcquireStats const& stats, ObserveFn const&
|
||||
// give-up path cannot fire, so an acquisition never ends.
|
||||
observe("acquire_deferrals", static_cast<std::int64_t>(stats.getDeferrals()));
|
||||
observe("acquire_timeouts", static_cast<std::int64_t>(stats.getTimeouts()));
|
||||
|
||||
// The same two events, narrowed to ledger acquisition. The pair above
|
||||
// sums every TimeoutCounter subclass, so a busy replay lane can imitate
|
||||
// a stalled ledger acquisition; compare these two instead when asking
|
||||
// whether ledger acquisition's give-up path is advancing.
|
||||
observe("acquire_ledger_deferrals", static_cast<std::int64_t>(stats.getLedgerDeferrals()));
|
||||
observe("acquire_ledger_timeouts", static_cast<std::int64_t>(stats.getLedgerTimeouts()));
|
||||
observe("acquire_give_ups", static_cast<std::int64_t>(stats.getGiveUps()));
|
||||
observe("acquire_aborts", static_cast<std::int64_t>(stats.getAborts()));
|
||||
observe("acquire_aborts_partial", static_cast<std::int64_t>(stats.getAbortsWithPartialWork()));
|
||||
|
||||
Reference in New Issue
Block a user