Skip to content

Commit 2d634ed

Browse files
marta-lokhovamarta-lokhova
committed
Last few bits of cleanup, fix reference bug
1 parent bffa6c7 commit 2d634ed

26 files changed

+113
-75
lines changed

‎src/bucket/BucketSnapshotManager.cpp

+33-17
Original file line numberDiff line numberDiff line change
@@ -52,44 +52,44 @@ BucketSnapshotManager::BucketSnapshotManager(
5252
}
5353
}
5454

55-
SearchableSnapshotConstPtr
56-
BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const
55+
template <class BucketT>
56+
std::map<uint32_t, SnapshotPtrT<BucketT>>
57+
copyHistoricalSnapshots(
58+
std::map<uint32_t, SnapshotPtrT<BucketT>> const& snapshots)
5759
{
58-
std::map<uint32_t, SnapshotPtrT<LiveBucket>> historicalSnapshots;
59-
for (auto const& [ledgerSeq, snap] : mLiveHistoricalSnapshots)
60+
std::map<uint32_t, SnapshotPtrT<BucketT>> copiedSnapshots;
61+
for (auto const& [ledgerSeq, snap] : snapshots)
6062
{
61-
historicalSnapshots.emplace(
63+
copiedSnapshots.emplace(
6264
ledgerSeq,
63-
std::make_unique<BucketListSnapshot<LiveBucket> const>(*snap));
65+
std::make_unique<BucketListSnapshot<BucketT> const>(*snap));
6466
}
67+
return copiedSnapshots;
68+
}
69+
70+
SearchableSnapshotConstPtr
71+
BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const
72+
{
6573
// Can't use std::make_shared due to private constructor
6674
return std::shared_ptr<SearchableLiveBucketListSnapshot>(
6775
new SearchableLiveBucketListSnapshot(
6876
*this,
6977
std::make_unique<BucketListSnapshot<LiveBucket>>(
7078
*mCurrLiveSnapshot),
71-
std::move(historicalSnapshots)));
79+
copyHistoricalSnapshots(mLiveHistoricalSnapshots)));
7280
}
7381

74-
std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>
82+
SearchableHotArchiveSnapshotConstPtr
7583
BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const
7684
{
7785
releaseAssert(mCurrHotArchiveSnapshot);
78-
std::map<uint32_t, SnapshotPtrT<HotArchiveBucket>> historicalSnapshots;
79-
for (auto const& [ledgerSeq, snap] : mHotArchiveHistoricalSnapshots)
80-
{
81-
historicalSnapshots.emplace(
82-
ledgerSeq,
83-
std::make_unique<BucketListSnapshot<HotArchiveBucket> const>(
84-
*snap));
85-
}
8686
// Can't use std::make_shared due to private constructor
8787
return std::shared_ptr<SearchableHotArchiveBucketListSnapshot>(
8888
new SearchableHotArchiveBucketListSnapshot(
8989
*this,
9090
std::make_unique<BucketListSnapshot<HotArchiveBucket>>(
9191
*mCurrHotArchiveSnapshot),
92-
std::move(historicalSnapshots)));
92+
copyHistoricalSnapshots(mHotArchiveHistoricalSnapshots)));
9393
}
9494

9595
medida::Timer&
@@ -132,6 +132,22 @@ BucketSnapshotManager::maybeCopySearchableBucketListSnapshot(
132132
}
133133
}
134134

135+
void
136+
BucketSnapshotManager::maybeCopySearchableHotArchiveBucketListSnapshot(
137+
SearchableHotArchiveSnapshotConstPtr& snapshot)
138+
{
139+
// The canonical snapshot held by the BucketSnapshotManager is not being
140+
// modified. Rather, a thread is checking it's copy against the canonical
141+
// snapshot, so use a shared lock.
142+
std::shared_lock<std::shared_mutex> lock(mSnapshotMutex);
143+
144+
if (!snapshot ||
145+
snapshot->getLedgerSeq() < mCurrHotArchiveSnapshot->getLedgerSeq())
146+
{
147+
snapshot = copySearchableHotArchiveBucketListSnapshot();
148+
}
149+
}
150+
135151
void
136152
BucketSnapshotManager::updateCurrentSnapshot(
137153
SnapshotPtrT<LiveBucket>&& liveSnapshot,

‎src/bucket/BucketSnapshotManager.h

+10-1
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ template <class BucketT>
3434
using SnapshotPtrT = std::unique_ptr<BucketListSnapshot<BucketT> const>;
3535
using SearchableSnapshotConstPtr =
3636
std::shared_ptr<SearchableLiveBucketListSnapshot const>;
37+
using SearchableHotArchiveSnapshotConstPtr =
38+
std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>;
3739

3840
// This class serves as the boundary between non-threadsafe singleton classes
3941
// (BucketManager, BucketList, Metrics, etc) and threadsafe, parallel BucketList
@@ -82,13 +84,20 @@ class BucketSnapshotManager : NonMovableOrCopyable
8284
SnapshotPtrT<HotArchiveBucket>&& hotArchiveSnapshot,
8385
uint32_t numHistoricalLedgers);
8486

87+
// Copy the most recent snapshot for the live bucket list
8588
SearchableSnapshotConstPtr copySearchableLiveBucketListSnapshot() const;
8689

87-
std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>
90+
// Copy the most recent snapshot for the hot archive bucket list
91+
SearchableHotArchiveSnapshotConstPtr
8892
copySearchableHotArchiveBucketListSnapshot() const;
8993

94+
// `maybeCopy` interface refreshes `snapshot` if a newer snapshot is
95+
// available. It's a no-op otherwise. This is useful to avoid unnecessary
96+
// copying.
9097
void
9198
maybeCopySearchableBucketListSnapshot(SearchableSnapshotConstPtr& snapshot);
99+
void maybeCopySearchableHotArchiveBucketListSnapshot(
100+
SearchableHotArchiveSnapshotConstPtr& snapshot);
92101

93102
// All metric recording functions must only be called by the main thread
94103
void startPointLoadTimer() const;

‎src/bucket/SearchableBucketList.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ class SearchableHotArchiveBucketListSnapshot
5353
std::vector<HotArchiveBucketEntry>
5454
loadKeys(std::set<LedgerKey, LedgerEntryIdCmp> const& inKeys) const;
5555

56-
friend std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>
56+
friend SearchableHotArchiveSnapshotConstPtr
5757
BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const;
5858
};
5959
}

‎src/bucket/test/BucketIndexTests.cpp

+17-15
Original file line numberDiff line numberDiff line change
@@ -803,19 +803,19 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
803803
BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION);
804804
addHotArchiveBatchAndUpdateSnapshot(*app, header, archivedEntries,
805805
restoredEntries, deletedEntries);
806-
searchableBL = app->getBucketManager()
807-
.getBucketSnapshotManager()
808-
.copySearchableHotArchiveBucketListSnapshot();
806+
app->getBucketManager()
807+
.getBucketSnapshotManager()
808+
.maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
809809
checkResult();
810810

811811
// Add a few batches so that entries are no longer in the top bucket
812812
for (auto i = 0; i < 100; ++i)
813813
{
814814
header.ledgerSeq += 1;
815815
addHotArchiveBatchAndUpdateSnapshot(*app, header, {}, {}, {});
816-
searchableBL = app->getBucketManager()
817-
.getBucketSnapshotManager()
818-
.copySearchableHotArchiveBucketListSnapshot();
816+
app->getBucketManager()
817+
.getBucketSnapshotManager()
818+
.maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
819819
}
820820

821821
// Shadow entries via liveEntry
@@ -825,9 +825,9 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
825825
header.ledgerSeq += 1;
826826
addHotArchiveBatchAndUpdateSnapshot(*app, header, {},
827827
{liveShadow1, liveShadow2}, {});
828-
searchableBL = app->getBucketManager()
829-
.getBucketSnapshotManager()
830-
.copySearchableHotArchiveBucketListSnapshot();
828+
app->getBucketManager()
829+
.getBucketSnapshotManager()
830+
.maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
831831

832832
// Point load
833833
for (auto const& k : {liveShadow1, liveShadow2})
@@ -847,9 +847,10 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
847847
header.ledgerSeq += 1;
848848
addHotArchiveBatchAndUpdateSnapshot(*app, header, {}, {},
849849
{deletedShadow});
850-
searchableBL = app->getBucketManager()
851-
.getBucketSnapshotManager()
852-
.copySearchableHotArchiveBucketListSnapshot();
850+
app->getBucketManager()
851+
.getBucketSnapshotManager()
852+
.maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
853+
853854
// Point load
854855
auto entryPtr = searchableBL->load(deletedShadow);
855856
REQUIRE(entryPtr);
@@ -870,9 +871,10 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
870871
header.ledgerSeq += 1;
871872
addHotArchiveBatchAndUpdateSnapshot(*app, header, {archivedShadow}, {},
872873
{});
873-
searchableBL = app->getBucketManager()
874-
.getBucketSnapshotManager()
875-
.copySearchableHotArchiveBucketListSnapshot();
874+
app->getBucketManager()
875+
.getBucketSnapshotManager()
876+
.maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
877+
876878
// Point load
877879
entryPtr = searchableBL->load(LedgerEntryKey(archivedShadow));
878880
REQUIRE(entryPtr);

‎src/bucket/test/BucketListTests.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -868,7 +868,8 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
868868
for_versions_from(20, *app, [&] {
869869
LedgerManagerForBucketTests& lm = app->getLedgerManager();
870870

871-
auto& networkConfig = app->getLedgerManager().getSorobanNetworkConfig();
871+
auto& networkConfig =
872+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
872873

873874
uint32_t windowSize = networkConfig.stateArchivalSettings()
874875
.bucketListSizeWindowSampleSize;

‎src/herder/HerderImpl.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -2101,7 +2101,8 @@ HerderImpl::maybeHandleUpgrade()
21012101
// no-op on any earlier protocol
21022102
return;
21032103
}
2104-
auto const& conf = mApp.getLedgerManager().getSorobanNetworkConfig();
2104+
auto const& conf =
2105+
mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
21052106

21062107
auto maybeNewMaxTxSize =
21072108
conf.txMaxSizeBytes() + getFlowControlExtraBuffer();
@@ -2153,7 +2154,7 @@ HerderImpl::start()
21532154
if (protocolVersionStartsFrom(version, SOROBAN_PROTOCOL_VERSION))
21542155
{
21552156
auto const& conf =
2156-
mApp.getLedgerManager().getSorobanNetworkConfig();
2157+
mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
21572158
mMaxTxSize = std::max(mMaxTxSize, conf.txMaxSizeBytes() +
21582159
getFlowControlExtraBuffer());
21592160
}

‎src/herder/TransactionQueue.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,8 @@ TransactionQueue::canAdd(
381381
auto txResult = tx->createSuccessResult();
382382
if (!tx->checkSorobanResourceAndSetError(
383383
mApp.getAppConnector(),
384-
mApp.getLedgerManager().getSorobanNetworkConfig(),
384+
mApp.getLedgerManager()
385+
.getSorobanNetworkConfigReadOnly(),
385386
mApp.getLedgerManager()
386387
.getLastClosedLedgerHeader()
387388
.header.ledgerVersion,

‎src/herder/test/HerderTests.cpp

+4-3
Original file line numberDiff line numberDiff line change
@@ -1007,7 +1007,8 @@ TEST_CASE("tx set hits overlay byte limit during construction",
10071007
cfg.mLedgerMaxInstructions = max;
10081008
});
10091009

1010-
auto const& conf = app->getLedgerManager().getSorobanNetworkConfig();
1010+
auto const& conf =
1011+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
10111012
uint32_t maxContractSize = 0;
10121013
maxContractSize = conf.maxContractSizeBytes();
10131014

@@ -1162,7 +1163,7 @@ TEST_CASE("surge pricing", "[herder][txset][soroban]")
11621163
auto tx = makeMultiPayment(acc1, root, 1, 100, 0, 1);
11631164

11641165
SorobanNetworkConfig conf =
1165-
app->getLedgerManager().getSorobanNetworkConfig();
1166+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
11661167

11671168
uint32_t const baseFee = 10'000'000;
11681169
SorobanResources resources;
@@ -3361,7 +3362,7 @@ TEST_CASE("soroban txs accepted by the network",
33613362
for (auto node : nodes)
33623363
{
33633364
REQUIRE(node->getLedgerManager()
3364-
.getSorobanNetworkConfig()
3365+
.getSorobanNetworkConfigReadOnly()
33653366
.ledgerMaxTxCount() == ledgerWideLimit * 10);
33663367
}
33673368

‎src/herder/test/TransactionQueueTests.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1259,7 +1259,7 @@ TEST_CASE("Soroban TransactionQueue limits",
12591259
auto account2 = root.create("a2", minBalance2);
12601260

12611261
SorobanNetworkConfig conf =
1262-
app->getLedgerManager().getSorobanNetworkConfig();
1262+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
12631263

12641264
SorobanResources resources;
12651265
resources.instructions = 2'000'000;

‎src/herder/test/TxSetTests.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1087,7 +1087,7 @@ TEST_CASE("txset nomination", "[txset]")
10871087
});
10881088

10891089
auto const& sorobanConfig =
1090-
app->getLedgerManager().getSorobanNetworkConfig();
1090+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
10911091
stellar::uniform_int_distribution<> txReadEntriesDistr(
10921092
1, sorobanConfig.txMaxReadLedgerEntries());
10931093

‎src/herder/test/UpgradesTests.cpp

+10-8
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ makeBucketListSizeWindowSampleSizeTestUpgrade(Application& app,
251251
{
252252
// Modify window size
253253
auto sas = app.getLedgerManager()
254-
.getSorobanNetworkConfig()
254+
.getSorobanNetworkConfigReadOnly()
255255
.stateArchivalSettings();
256256
sas.bucketListSizeWindowSampleSize = newWindowSize;
257257

@@ -837,7 +837,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
837837
executeUpgrade(*app, makeProtocolVersionUpgrade(
838838
static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION)));
839839
auto const& sorobanConfig =
840-
app->getLedgerManager().getSorobanNetworkConfig();
840+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
841841
SECTION("unknown config upgrade set is ignored")
842842
{
843843
auto contractID = autocheck::generator<Hash>()(5);
@@ -905,7 +905,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
905905
auto const newSize = 20;
906906
populateValuesAndUpgradeSize(newSize);
907907
auto const& cfg2 =
908-
app->getLedgerManager().getSorobanNetworkConfig();
908+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
909909

910910
// Verify that we popped the 10 oldest values
911911
auto sum = 0;
@@ -927,7 +927,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
927927
auto const newSize = 40;
928928
populateValuesAndUpgradeSize(newSize);
929929
auto const& cfg2 =
930-
app->getLedgerManager().getSorobanNetworkConfig();
930+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
931931

932932
// Verify that we backfill 10 copies of the oldest value
933933
auto sum = 0;
@@ -957,7 +957,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
957957
LedgerTxn ltx2(app->getLedgerTxnRoot());
958958

959959
auto const& cfg =
960-
app->getLedgerManager().getSorobanNetworkConfig();
960+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
961961
initialSize =
962962
cfg.mStateArchivalSettings.bucketListSizeWindowSampleSize;
963963
initialWindow = cfg.mBucketListSizeSnapshots;
@@ -972,7 +972,8 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
972972
REQUIRE(configUpgradeSet);
973973
executeUpgrade(*app, makeConfigUpgrade(*configUpgradeSet));
974974

975-
auto const& cfg = app->getLedgerManager().getSorobanNetworkConfig();
975+
auto const& cfg =
976+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
976977
REQUIRE(cfg.mStateArchivalSettings.bucketListSizeWindowSampleSize ==
977978
initialSize);
978979
REQUIRE(cfg.mBucketListSizeSnapshots == initialWindow);
@@ -1086,7 +1087,7 @@ TEST_CASE("Soroban max tx set size upgrade applied to ledger",
10861087
static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION)));
10871088

10881089
auto const& sorobanConfig =
1089-
app->getLedgerManager().getSorobanNetworkConfig();
1090+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
10901091

10911092
executeUpgrade(*app, makeMaxSorobanTxSizeUpgrade(123));
10921093
REQUIRE(sorobanConfig.ledgerMaxTxCount() == 123);
@@ -2245,7 +2246,8 @@ TEST_CASE("configuration initialized in version upgrade", "[upgrades]")
22452246
InitialSorobanNetworkConfig::MAX_CONTRACT_SIZE);
22462247

22472248
// Check that BucketList size window initialized with current BL size
2248-
auto& networkConfig = app->getLedgerManager().getSorobanNetworkConfig();
2249+
auto& networkConfig =
2250+
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
22492251
REQUIRE(networkConfig.getAverageBucketListSize() == blSize);
22502252

22512253
// Check in memory window

‎src/ledger/LedgerManager.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ class LedgerManager
131131
// The config is automatically refreshed on protocol upgrades.
132132
// Ledger txn here is needed for the sake of lazy load; it won't be
133133
// used most of the time.
134-
virtual SorobanNetworkConfig const& getSorobanNetworkConfig() = 0;
134+
virtual SorobanNetworkConfig const& getSorobanNetworkConfigReadOnly() = 0;
135135
virtual SorobanNetworkConfig const& getSorobanNetworkConfigForApply() = 0;
136136

137137
virtual bool hasSorobanNetworkConfig() const = 0;

‎src/ledger/LedgerManagerImpl.cpp

+5-4
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ LedgerManagerImpl::maxLedgerResources(bool isSoroban)
453453

454454
if (isSoroban)
455455
{
456-
auto conf = getSorobanNetworkConfig();
456+
auto conf = getSorobanNetworkConfigReadOnly();
457457
std::vector<int64_t> limits = {conf.ledgerMaxTxCount(),
458458
conf.ledgerMaxInstructions(),
459459
conf.ledgerMaxTransactionSizesBytes(),
@@ -475,7 +475,8 @@ LedgerManagerImpl::maxSorobanTransactionResources()
475475
{
476476
ZoneScoped;
477477

478-
auto const& conf = mApp.getLedgerManager().getSorobanNetworkConfig();
478+
auto const& conf =
479+
mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
479480
int64_t const opCount = 1;
480481
std::vector<int64_t> limits = {opCount,
481482
conf.txMaxInstructions(),
@@ -540,7 +541,7 @@ LedgerManagerImpl::getLastClosedLedgerNum() const
540541
}
541542

542543
SorobanNetworkConfig const&
543-
LedgerManagerImpl::getSorobanNetworkConfig()
544+
LedgerManagerImpl::getSorobanNetworkConfigReadOnly()
544545
{
545546
releaseAssert(threadIsMain());
546547
releaseAssert(hasSorobanNetworkConfig());
@@ -1759,7 +1760,7 @@ LedgerManagerImpl::ledgerClosed(
17591760
protocolVersionStartsFrom(initialLedgerVers, SOROBAN_PROTOCOL_VERSION))
17601761
{
17611762
ledgerCloseMeta->setNetworkConfiguration(
1762-
getSorobanNetworkConfig(),
1763+
getSorobanNetworkConfigReadOnly(),
17631764
mApp.getConfig().EMIT_LEDGER_CLOSE_META_EXT_V1);
17641765
}
17651766

0 commit comments

Comments
 (0)