Skip to content

Commit 3b8a91e

Browse files
SirTysonSirTyson
committed
Threadsafe EvictionStatistics
1 parent 4e94bc4 commit 3b8a91e

11 files changed

+81
-51
lines changed

‎src/bucket/Bucket.cpp

+5-7
Original file line numberDiff line numberDiff line change
@@ -688,9 +688,11 @@ Bucket::scanForEvictionLegacy(AbstractLedgerTxn& ltx, EvictionIterator& iter,
688688
uint32_t ledgerSeq,
689689
medida::Counter& entriesEvictedCounter,
690690
medida::Counter& bytesScannedForEvictionCounter,
691-
std::optional<EvictionStatistics>& stats) const
691+
std::shared_ptr<EvictionStatistics> stats) const
692692
{
693693
ZoneScoped;
694+
releaseAssert(stats);
695+
694696
if (isEmpty() ||
695697
protocolVersionIsBefore(getBucketVersion(shared_from_this()),
696698
SOROBAN_PROTOCOL_VERSION))
@@ -739,12 +741,8 @@ Bucket::scanForEvictionLegacy(AbstractLedgerTxn& ltx, EvictionIterator& iter,
739741
if (shouldEvict())
740742
{
741743
ZoneNamedN(evict, "evict entry", true);
742-
if (stats.has_value())
743-
{
744-
++stats->numEntriesEvicted;
745-
stats->evictedEntriesAgeSum +=
746-
ledgerSeq - liveUntilLedger;
747-
}
744+
auto age = ledgerSeq - liveUntilLedger;
745+
stats->recordEvictedEntry(age);
748746

749747
ltx.erase(ttlKey);
750748
ltx.erase(LedgerEntryKey(le));

‎src/bucket/Bucket.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ class Bucket : public std::enable_shared_from_this<Bucket>,
138138
uint32_t ledgerSeq,
139139
medida::Counter& entriesEvictedCounter,
140140
medida::Counter& bytesScannedForEvictionCounter,
141-
std::optional<EvictionStatistics>& stats) const;
141+
std::shared_ptr<EvictionStatistics> stats) const;
142142

143143
bool scanForEviction(EvictionIterator& iter, uint32_t& bytesToScan,
144144
uint32_t ledgerSeq,

‎src/bucket/BucketList.cpp

+8-19
Original file line numberDiff line numberDiff line change
@@ -686,8 +686,10 @@ bool
686686
BucketList::updateEvictionIterAndRecordStats(
687687
EvictionIterator& iter, EvictionIterator startIter,
688688
uint32_t configFirstScanLevel, uint32_t ledgerSeq,
689-
std::optional<EvictionStatistics>& stats, EvictionCounters& counters)
689+
std::shared_ptr<EvictionStatistics> stats, EvictionCounters& counters)
690690
{
691+
releaseAssert(stats);
692+
691693
// If we reached eof in curr bucket, start scanning snap.
692694
// Last level has no snap so cycle back to the initial level.
693695
if (iter.isCurrBucket && iter.bucketListLevel != kNumLevels - 1)
@@ -708,23 +710,8 @@ BucketList::updateEvictionIterAndRecordStats(
708710
{
709711
iter.bucketListLevel = configFirstScanLevel;
710712

711-
// If eviction metrics are not null, we have accounted for a
712-
// complete cycle and should log the metrics
713-
if (stats)
714-
{
715-
counters.evictionCyclePeriod.set_count(
716-
ledgerSeq - stats->evictionCycleStartLedger);
717-
718-
auto averageAge = stats->numEntriesEvicted == 0
719-
? 0
720-
: stats->evictedEntriesAgeSum /
721-
stats->numEntriesEvicted;
722-
counters.averageEvictedEntryAge.set_count(averageAge);
723-
}
724-
725-
// Reset metrics at beginning of new eviction cycle
726-
stats = std::make_optional<EvictionStatistics>();
727-
stats->evictionCycleStartLedger = ledgerSeq;
713+
// Record then reset metrics at beginning of new eviction cycle
714+
stats->submitMetricsAndRestartCycle(ledgerSeq, counters);
728715
}
729716
}
730717

@@ -763,8 +750,10 @@ void
763750
BucketList::scanForEvictionLegacy(Application& app, AbstractLedgerTxn& ltx,
764751
uint32_t ledgerSeq,
765752
EvictionCounters& counters,
766-
std::optional<EvictionStatistics>& stats)
753+
std::shared_ptr<EvictionStatistics> stats)
767754
{
755+
releaseAssert(stats);
756+
768757
auto getBucketFromIter = [&levels = mLevels](EvictionIterator const& iter) {
769758
auto& level = levels.at(iter.bucketListLevel);
770759
return iter.isCurrBucket ? level.getCurr() : level.getSnap();

‎src/bucket/BucketList.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,7 @@ class BucketList
474474
static bool updateEvictionIterAndRecordStats(
475475
EvictionIterator& iter, EvictionIterator startIter,
476476
uint32_t configFirstScanLevel, uint32_t ledgerSeq,
477-
std::optional<EvictionStatistics>& stats, EvictionCounters& counters);
477+
std::shared_ptr<EvictionStatistics> stats, EvictionCounters& counters);
478478

479479
static void checkIfEvictionScanIsStuck(EvictionIterator const& evictionIter,
480480
uint32_t scanSize,
@@ -483,7 +483,7 @@ class BucketList
483483

484484
void scanForEvictionLegacy(Application& app, AbstractLedgerTxn& ltx,
485485
uint32_t ledgerSeq, EvictionCounters& counters,
486-
std::optional<EvictionStatistics>& stats);
486+
std::shared_ptr<EvictionStatistics> stats);
487487

488488
// Restart any merges that might be running on background worker threads,
489489
// merging buckets between levels. This needs to be called after forcing a

‎src/bucket/BucketListSnapshot.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,11 @@ SearchableBucketListSnapshot::loopAllBuckets(
7070
EvictionResult
7171
SearchableBucketListSnapshot::scanForEviction(
7272
uint32_t ledgerSeq, EvictionCounters& counters,
73-
EvictionIterator evictionIter, std::optional<EvictionStatistics>& stats,
73+
EvictionIterator evictionIter, std::shared_ptr<EvictionStatistics> stats,
7474
StateArchivalSettings const& sas)
7575
{
7676
releaseAssert(mSnapshot);
77+
releaseAssert(stats);
7778

7879
auto getBucketFromIter =
7980
[&levels = mSnapshot->getLevels()](

‎src/bucket/BucketListSnapshot.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ class SearchableBucketListSnapshot : public NonMovableOrCopyable
9191
EvictionResult scanForEviction(uint32_t ledgerSeq,
9292
EvictionCounters& counters,
9393
EvictionIterator evictionIter,
94-
std::optional<EvictionStatistics>& stats,
94+
std::shared_ptr<EvictionStatistics> stats,
9595
StateArchivalSettings const& sas);
9696
};
9797
}

‎src/bucket/BucketManager.h

+20-9
Original file line numberDiff line numberDiff line change
@@ -125,15 +125,6 @@ struct EvictionResult
125125
StateArchivalSettings const& currSas) const;
126126
};
127127

128-
struct EvictionStatistics
129-
{
130-
// Evicted entry "age" is the delta between its liveUntilLedger and the
131-
// ledger when the entry is actually evicted
132-
uint64_t evictedEntriesAgeSum{};
133-
uint64_t numEntriesEvicted{};
134-
uint32_t evictionCycleStartLedger{};
135-
};
136-
137128
struct EvictionCounters
138129
{
139130
medida::Counter& entriesEvicted;
@@ -145,6 +136,26 @@ struct EvictionCounters
145136
EvictionCounters(Application& app);
146137
};
147138

139+
class EvictionStatistics
140+
{
141+
private:
142+
std::mutex mLock{};
143+
144+
// Only record metrics if we've seen a complete cycle to avoid noise
145+
bool mCompleteCycle{false};
146+
uint64_t mEvictedEntriesAgeSum{};
147+
uint64_t mNumEntriesEvicted{};
148+
uint32_t mEvictionCycleStartLedger{};
149+
150+
public:
151+
// Evicted entry "age" is the delta between its liveUntilLedger and the
152+
// ledger when the entry is actually evicted
153+
void recordEvictedEntry(uint64_t age);
154+
155+
void submitMetricsAndRestartCycle(uint32_t currLedgerSeq,
156+
EvictionCounters& counters);
157+
};
158+
148159
/**
149160
* BucketManager is responsible for maintaining a collection of Buckets of
150161
* ledger entries (each sorted, de-duplicated and identified by hash) and,

‎src/bucket/BucketManagerImpl.cpp

+39-8
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,39 @@
4343
namespace stellar
4444
{
4545

46+
void
47+
EvictionStatistics::recordEvictedEntry(uint64_t age)
48+
{
49+
std::lock_guard l(mLock);
50+
++mNumEntriesEvicted;
51+
mEvictedEntriesAgeSum += age;
52+
}
53+
54+
void
55+
EvictionStatistics::submitMetricsAndRestartCycle(uint32_t currLedgerSeq,
56+
EvictionCounters& counters)
57+
{
58+
std::lock_guard l(mLock);
59+
60+
// Only record metrics if we've seen a complete cycle to avoid noise
61+
if (mCompleteCycle)
62+
{
63+
counters.evictionCyclePeriod.set_count(currLedgerSeq -
64+
mEvictionCycleStartLedger);
65+
66+
auto averageAge = mNumEntriesEvicted == 0
67+
? 0
68+
: mEvictedEntriesAgeSum / mNumEntriesEvicted;
69+
counters.averageEvictedEntryAge.set_count(averageAge);
70+
}
71+
72+
// Reset to start new cycle
73+
mCompleteCycle = true;
74+
mEvictedEntriesAgeSum = 0;
75+
mNumEntriesEvicted = 0;
76+
mEvictionCycleStartLedger = currLedgerSeq;
77+
}
78+
4679
std::unique_ptr<BucketManager>
4780
BucketManager::create(Application& app)
4881
{
@@ -147,6 +180,7 @@ BucketManagerImpl::BucketManagerImpl(Application& app)
147180
, mBucketListSizeCounter(
148181
app.getMetrics().NewCounter({"bucketlist", "size", "bytes"}))
149182
, mBucketListEvictionCounters(app)
183+
, mEvictionStatistics(std::make_shared<EvictionStatistics>())
150184
// Minimal DB is stored in the buckets dir, so delete it only when
151185
// mode does not use minimal DB
152186
, mDeleteEntireBucketDirInDtor(
@@ -958,6 +992,7 @@ BucketManagerImpl::startBackgroundEvictionScan(uint32_t ledgerSeq)
958992
releaseAssert(mApp.getConfig().isUsingBucketListDB());
959993
releaseAssert(mSnapshotManager);
960994
releaseAssert(!mEvictionFuture.valid());
995+
releaseAssert(mEvictionStatistics);
961996

962997
auto searchableBL = mSnapshotManager->getSearchableBucketListSnapshot();
963998
auto const& cfg = mApp.getLedgerManager().getSorobanNetworkConfig();
@@ -966,8 +1001,7 @@ BucketManagerImpl::startBackgroundEvictionScan(uint32_t ledgerSeq)
9661001
using task_t = std::packaged_task<EvictionResult()>;
9671002
auto task = std::make_shared<task_t>(
9681003
[bl = move(searchableBL), iter = cfg.evictionIterator(), ledgerSeq, sas,
969-
&counters = mBucketListEvictionCounters,
970-
&stats = mEvictionStatistics] {
1004+
&counters = mBucketListEvictionCounters, stats = mEvictionStatistics] {
9711005
return bl->scanForEviction(ledgerSeq, counters, iter, stats, sas);
9721006
});
9731007

@@ -984,6 +1018,7 @@ BucketManagerImpl::resolveBackgroundEvictionScan(
9841018
{
9851019
ZoneScoped;
9861020
releaseAssert(threadIsMain());
1021+
releaseAssert(mEvictionStatistics);
9871022

9881023
if (!mEvictionFuture.valid())
9891024
{
@@ -1032,13 +1067,9 @@ BucketManagerImpl::resolveBackgroundEvictionScan(
10321067
ltx.erase(getTTLKey(entryToEvictIter->key));
10331068
--remainingEntriesToEvict;
10341069

1070+
auto age = ledgerSeq - entryToEvictIter->liveUntilLedger;
1071+
mEvictionStatistics->recordEvictedEntry(age);
10351072
mBucketListEvictionCounters.entriesEvicted.inc();
1036-
if (mEvictionStatistics.has_value())
1037-
{
1038-
++mEvictionStatistics->numEntriesEvicted;
1039-
mEvictionStatistics->evictedEntriesAgeSum +=
1040-
ledgerSeq - entryToEvictIter->liveUntilLedger;
1041-
}
10421073

10431074
newEvictionIterator = entryToEvictIter->iter;
10441075
entryToEvictIter = eligibleKeys.erase(entryToEvictIter);

‎src/bucket/BucketManagerImpl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class BucketManagerImpl : public BucketManager
5858
medida::Counter& mBucketListSizeCounter;
5959
EvictionCounters mBucketListEvictionCounters;
6060
MergeCounters mMergeCounters;
61-
std::optional<EvictionStatistics> mEvictionStatistics{};
61+
std::shared_ptr<EvictionStatistics> mEvictionStatistics{};
6262

6363
std::future<EvictionResult> mEvictionFuture{};
6464

‎test-tx-meta-baseline-current/InvokeHostFunctionTests.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1375,7 +1375,7 @@
13751375
"bKDF6V5IzTo=",
13761376
"bKDF6V5IzTo="
13771377
],
1378-
"temp entry eviction" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ],
1378+
"temp entry eviction|sql" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ],
13791379
"transaction validation diagnostics" : [ "bKDF6V5IzTo=" ],
13801380
"version test" : [ "766L+TYsWqA=" ]
13811381
}

‎test-tx-meta-baseline-next/InvokeHostFunctionTests.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -1379,7 +1379,7 @@
13791379
"bKDF6V5IzTo=",
13801380
"bKDF6V5IzTo="
13811381
],
1382-
"temp entry eviction" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ],
1382+
"temp entry eviction|sql" : [ "bKDF6V5IzTo=", "bKDF6V5IzTo=" ],
13831383
"transaction validation diagnostics" : [ "bKDF6V5IzTo=" ],
13841384
"version test" : [ "766L+TYsWqA=" ]
13851385
}

0 commit comments

Comments
 (0)