Skip to content

Commit 720aff0

Browse files
SirTysonSirTyson
authored
Avoid redundant eviction disk reads in p23 (#4979)
# Description Resolves stellar/stellar-core-internal#402 Avoids redundant disk IO before protocol 24 in the eviction scan. # Checklist - [x] Reviewed the [contributing](https://github.com/stellar/stellar-core/blob/master/CONTRIBUTING.md#submitting-changes) document - [x] Rebased on top of master (no merge commits) - [x] Ran `clang-format` v8.0.0 (via `make format` or the Visual Studio extension) - [x] Compiles - [x] Ran all tests - [ ] If change impacts performance, include supporting evidence per the [performance document](https://github.com/stellar/stellar-core/blob/master/performance-eval/performance-eval.md)
2 parents 9ec1c10 + d3b343e commit 720aff0

File tree

3 files changed

+260
-48
lines changed

3 files changed

+260
-48
lines changed

‎src/bucket/BucketSnapshot.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,8 +314,11 @@ LiveBucketSnapshot::scanForEviction(
314314
// for persistent entries, we need to make sure we evict the
315315
// newest version of the entry, since it will be persisted in
316316
// the hot archive. So we add persistent entries to our DB
317-
// lookup to find the newest version.
318-
if (isPersistentEntry(le.data))
317+
// lookup to find the newest version. Note that there was a
318+
// bug in protocol 23 where this check was not performed.
319+
if (isPersistentEntry(le.data) &&
320+
protocolVersionStartsFrom(ledgerVers,
321+
ProtocolVersion::V_24))
319322
{
320323
keysToSearch.emplace(LedgerEntryKey(le));
321324
}

‎src/bucket/test/BucketListTests.cpp‎

Lines changed: 120 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1550,61 +1550,135 @@ TEST_CASE_VERSIONS("eviction scan", "[bucketlist][archival][soroban]")
15501550
closeLedger(*app);
15511551
}
15521552

1553-
updateStateArchivalSettings(
1554-
[&](StateArchivalSettings& sa) {
1555-
// Scan meta entry + one other entry in initial scan
1556-
sa.evictionScanSize = metadataSize + 1;
1557-
// Reset eviction iter start of bucket being tested
1558-
sa.startingEvictionScanLevel = levelToTest;
1559-
},
1560-
[&](EvictionIterator& ei) {
1561-
ei.bucketFileOffset = 0;
1562-
ei.isCurrBucket = isCurr;
1563-
ei.bucketListLevel = 1;
1564-
});
1565-
1566-
// Advance until one ledger before bucket is updated
1567-
auto ledgersUntilUpdate =
1568-
LiveBucketList::bucketUpdatePeriod(levelToTest,
1569-
isCurr) -
1570-
1; // updateStateArchivalSettings closes a ledger that we
1571-
// need to count
1572-
REQUIRE(ledgersUntilUpdate >= 1);
1573-
for (uint32_t i = 0; i < ledgersUntilUpdate - 1; ++i)
1574-
{
1575-
auto startingIter = evictionIter();
1553+
auto f = [&](bool disableEviction) {
1554+
updateStateArchivalSettings(
1555+
[&](StateArchivalSettings& sa) {
1556+
// Scan meta entry + one other entry in initial
1557+
// scan, or disable scan entirely
1558+
sa.evictionScanSize =
1559+
disableEviction ? 0 : metadataSize + 1;
1560+
// Reset eviction iter start of bucket being tested
1561+
sa.startingEvictionScanLevel = levelToTest;
1562+
},
1563+
[&](EvictionIterator& ei) {
1564+
// If scanning is enabled, reset iterator to the
1565+
// start. Otherwise, set the iterator to some
1566+
// arbitrary location so we can see if it was
1567+
// changed properly.
1568+
ei.bucketFileOffset = disableEviction ? 10 : 0;
1569+
ei.isCurrBucket = isCurr;
1570+
ei.bucketListLevel = 1;
1571+
});
1572+
1573+
// Advance until one ledger before bucket is updated
1574+
auto ledgersUntilUpdate =
1575+
LiveBucketList::bucketUpdatePeriod(levelToTest,
1576+
isCurr) -
1577+
1; // updateStateArchivalSettings closes a ledger that
1578+
// we need to count
1579+
REQUIRE(ledgersUntilUpdate >= 1);
1580+
for (uint32_t i = 0; i < ledgersUntilUpdate - 1; ++i)
1581+
{
1582+
auto startingIter = evictionIter();
1583+
closeLedger(*app);
1584+
++ledgerSeq;
1585+
1586+
if (disableEviction)
1587+
{
1588+
// Make sure scan doesn't advance iterator when
1589+
// disabled
1590+
REQUIRE(evictionIter() == startingIter);
1591+
}
1592+
else
1593+
{
1594+
// Check that iterator is making progress correctly
1595+
REQUIRE(evictionIter().bucketFileOffset >
1596+
startingIter.bucketFileOffset);
1597+
REQUIRE(evictionIter().bucketListLevel ==
1598+
levelToTest);
1599+
REQUIRE(evictionIter().isCurrBucket == isCurr);
1600+
}
1601+
}
1602+
1603+
auto iterBeforeBucketChange = evictionIter();
1604+
1605+
// Next ledger close should update bucket
1606+
auto startingHash = bucket()->getHash();
1607+
closeLedger(*app);
1608+
++ledgerSeq;
1609+
1610+
// Check that bucket actually changed
1611+
REQUIRE(bucket()->getHash() != startingHash);
1612+
1613+
// The iterator retroactively checks if the Bucket has
1614+
// changed, so close one additional ledger to check if the
1615+
// iterator has reset. This is due to the scan using a
1616+
// BucketList snapshot of the lcl ledger.
15761617
closeLedger(*app);
15771618
++ledgerSeq;
15781619

1579-
// Check that iterator is making progress correctly
1580-
REQUIRE(evictionIter().bucketFileOffset >
1581-
startingIter.bucketFileOffset);
1620+
LiveBucketInputIterator in(bucket());
1621+
1622+
// Check that iterator has reset to beginning of bucket
1623+
if (disableEviction)
1624+
{
1625+
REQUIRE(evictionIter().bucketFileOffset == 0);
1626+
}
1627+
else
1628+
{
1629+
// If scan is enabled, we should reset then scan the
1630+
// first entry (not including meta)
1631+
REQUIRE(evictionIter().bucketFileOffset ==
1632+
metadataSize + xdr::xdr_size(*in) +
1633+
xdrOverheadBytes);
1634+
}
15821635
REQUIRE(evictionIter().bucketListLevel == levelToTest);
15831636
REQUIRE(evictionIter().isCurrBucket == isCurr);
1584-
}
15851637

1586-
// Next ledger close should update bucket
1587-
auto startingHash = bucket()->getHash();
1588-
closeLedger(*app);
1589-
++ledgerSeq;
1638+
// Check that the iterator actually changed
1639+
REQUIRE(!(iterBeforeBucketChange == evictionIter()));
15901640

1591-
// Check that bucket actually changed
1592-
REQUIRE(bucket()->getHash() != startingHash);
1593-
1594-
// The iterator retroactively checks if the Bucket has
1595-
// changed, so close one additional ledger to check if the
1596-
// iterator has reset
1597-
closeLedger(*app);
1598-
++ledgerSeq;
1641+
// If eviction was disabled, re-enable it and verify we
1642+
// start making progress again
1643+
if (disableEviction)
1644+
{
1645+
// Re-enable eviction scan
1646+
updateStateArchivalSettings(
1647+
[&](StateArchivalSettings& sa) {
1648+
sa.evictionScanSize = metadataSize + 1;
1649+
},
1650+
[](EvictionIterator&) {
1651+
// Don't modify iterator, let it continue from
1652+
// where it reset to
1653+
});
1654+
1655+
// Close a few ledgers and verify iterator advances
1656+
for (uint32_t i = 0; i < 2; ++i)
1657+
{
1658+
auto prevIter = evictionIter();
1659+
closeLedger(*app);
1660+
++ledgerSeq;
1661+
1662+
// Iterator should be making progress now that
1663+
// eviction is re-enabled
1664+
REQUIRE(evictionIter().bucketFileOffset >
1665+
prevIter.bucketFileOffset);
1666+
REQUIRE(evictionIter().bucketListLevel ==
1667+
levelToTest);
1668+
REQUIRE(evictionIter().isCurrBucket == isCurr);
1669+
}
1670+
}
1671+
};
15991672

1600-
LiveBucketInputIterator in(bucket());
1673+
SECTION("eviction disabled")
1674+
{
1675+
f(true);
1676+
}
16011677

1602-
// Check that iterator has reset to beginning of bucket and
1603-
// read meta entry + one additional entry
1604-
REQUIRE(evictionIter().bucketFileOffset ==
1605-
metadataSize + xdr::xdr_size(*in) + xdrOverheadBytes);
1606-
REQUIRE(evictionIter().bucketListLevel == levelToTest);
1607-
REQUIRE(evictionIter().isCurrBucket == isCurr);
1678+
SECTION("eviction enabled")
1679+
{
1680+
f(false);
1681+
}
16081682
};
16091683

16101684
SECTION("curr bucket")

‎src/transactions/test/InvokeHostFunctionTests.cpp‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7501,6 +7501,141 @@ TEST_CASE_VERSIONS("do not evict outdated keys", "[archival][soroban]")
75017501
});
75027502
}
75037503

7504+
TEST_CASE("disable eviction scan", "[archival][soroban]")
7505+
{
7506+
// This test verifies that when the eviction scan size is set to 0,
7507+
// no entries are evicted and the eviction iterator does not advance.
7508+
auto cfg = getTestConfig(0);
7509+
cfg.TESTING_SOROBAN_HIGH_LIMIT_OVERRIDE = true;
7510+
cfg.TESTING_MINIMUM_PERSISTENT_ENTRY_LIFETIME = 10;
7511+
cfg.OVERRIDE_EVICTION_PARAMS_FOR_TESTING = true;
7512+
cfg.TESTING_STARTING_EVICTION_SCAN_LEVEL = 1;
7513+
cfg.TESTING_MAX_ENTRIES_TO_ARCHIVE = 1;
7514+
7515+
SorobanTest test(cfg, false);
7516+
ContractStorageTestClient client(test);
7517+
auto& snapshotManager =
7518+
test.getApp().getBucketManager().getBucketSnapshotManager();
7519+
7520+
// WASM and instance should not expire
7521+
test.invokeExtendOp(client.getContract().getKeys(), 10'000);
7522+
7523+
// We'll create two evictable entries, one persistent and one temporary.
7524+
auto persistentKey = client.getContract().getDataKey(
7525+
makeSymbolSCVal("persistent_key"), ContractDataDurability::PERSISTENT);
7526+
client.put("persistent_key", ContractDataDurability::PERSISTENT, 123);
7527+
7528+
auto firstEvictionLedger =
7529+
test.getLCLSeq() +
7530+
MinimumSorobanNetworkConfig::MINIMUM_PERSISTENT_ENTRY_LIFETIME;
7531+
7532+
auto temporaryKey = client.getContract().getDataKey(
7533+
makeSymbolSCVal("temporary_key"), ContractDataDurability::TEMPORARY);
7534+
client.put("temporary_key", ContractDataDurability::TEMPORARY, 456);
7535+
7536+
// modifySorobanNetworkConfig will close 4 ledgers before the upgrade will
7537+
// take affect, so close enough ledgers here such that the persistent entry
7538+
// would be evicted on the ledger immediately following the upgrade.
7539+
for (auto ledgerSeq = test.getLCLSeq() + 1;
7540+
ledgerSeq < firstEvictionLedger - 4; ++ledgerSeq)
7541+
{
7542+
closeLedgerOn(test.getApp(), ledgerSeq, 2, 1, 2016);
7543+
}
7544+
7545+
// Disable eviction scan by setting evictionScanSize to 0
7546+
modifySorobanNetworkConfig(test.getApp(), [](SorobanNetworkConfig& cfg) {
7547+
cfg.mStateArchivalSettings.evictionScanSize = 0;
7548+
});
7549+
7550+
// Capture the eviction iterator after disabling eviction scan
7551+
auto initialIterator = test.getNetworkCfg().evictionIterator();
7552+
7553+
// Close ledgers well beyond when the entries would have been evicted.
7554+
auto closeLedgersUntil = test.getLCLSeq() + 20;
7555+
for (auto ledgerSeq = test.getLCLSeq() + 1; ledgerSeq <= closeLedgersUntil;
7556+
++ledgerSeq)
7557+
{
7558+
closeLedgerOn(test.getApp(), ledgerSeq, 2, 1, 2016);
7559+
7560+
// Verify iterator has not changed
7561+
REQUIRE(initialIterator == test.getNetworkCfg().evictionIterator());
7562+
}
7563+
7564+
auto liveBL = snapshotManager.copySearchableLiveBucketListSnapshot();
7565+
auto hotArchive =
7566+
snapshotManager.copySearchableHotArchiveBucketListSnapshot();
7567+
7568+
auto assertTemp = [&](bool isLive) {
7569+
liveBL = snapshotManager.copySearchableLiveBucketListSnapshot();
7570+
hotArchive =
7571+
snapshotManager.copySearchableHotArchiveBucketListSnapshot();
7572+
auto tempLiveLoad = liveBL->load(temporaryKey);
7573+
REQUIRE(static_cast<bool>(tempLiveLoad) == isLive);
7574+
7575+
// Temp entries are never archived
7576+
REQUIRE(!hotArchive->load(temporaryKey));
7577+
};
7578+
7579+
auto assertPersistent = [&](bool isLive) {
7580+
liveBL = snapshotManager.copySearchableLiveBucketListSnapshot();
7581+
hotArchive =
7582+
snapshotManager.copySearchableHotArchiveBucketListSnapshot();
7583+
7584+
auto persistentLiveLoad = liveBL->load(persistentKey);
7585+
REQUIRE(static_cast<bool>(persistentLiveLoad) == isLive);
7586+
7587+
auto hotArchiveLoad = hotArchive->load(persistentKey);
7588+
REQUIRE(static_cast<bool>(hotArchiveLoad) != isLive);
7589+
};
7590+
7591+
// Verify entries are not evicted
7592+
assertPersistent(true);
7593+
assertTemp(true);
7594+
7595+
// Now, re-enable eviction. maxEntriesToArchive = 1 limits archiving to
7596+
// one entry per ledger. Note that the first eviction will actually occur on
7597+
// the upgrade ledger itself.
7598+
modifySorobanNetworkConfig(test.getApp(), [](SorobanNetworkConfig& cfg) {
7599+
cfg.mStateArchivalSettings.evictionScanSize = 10000;
7600+
cfg.mStateArchivalSettings.maxEntriesToArchive = 1;
7601+
});
7602+
7603+
auto iteratorAfterUpgrade = test.getNetworkCfg().evictionIterator();
7604+
7605+
// Iterator should have advanced from initial position during the upgrade
7606+
// ledger.
7607+
REQUIRE(!(initialIterator == iteratorAfterUpgrade));
7608+
initialIterator = iteratorAfterUpgrade;
7609+
7610+
// Check that exactly one entry has been evicted
7611+
liveBL = snapshotManager.copySearchableLiveBucketListSnapshot();
7612+
hotArchive = snapshotManager.copySearchableHotArchiveBucketListSnapshot();
7613+
7614+
auto persistentLiveLoad = liveBL->load(persistentKey);
7615+
if (persistentLiveLoad)
7616+
{
7617+
// If perstent entry is live, assert that the temp entry is evicted.
7618+
assertPersistent(true);
7619+
assertTemp(false);
7620+
}
7621+
else
7622+
{
7623+
// If perstent entry is evicted, assert that the temp entry is live.
7624+
assertPersistent(false);
7625+
assertTemp(true);
7626+
}
7627+
7628+
// Close one more ledger to evict the last remaining entry.
7629+
closeLedgerOn(test.getApp(), test.getLCLSeq() + 1, 2, 1, 2016);
7630+
7631+
// check that the iterator has advanced
7632+
REQUIRE(!(initialIterator == test.getNetworkCfg().evictionIterator()));
7633+
7634+
// Verify both entries have been evicted
7635+
assertPersistent(false);
7636+
assertTemp(false);
7637+
}
7638+
75047639
TEST_CASE("Module cache cost with restore gaps", "[tx][soroban][modulecache]")
75057640
{
75067641
VirtualClock clock;

0 commit comments

Comments
 (0)