Evict Temporary Entries #3874

SirTyson · 2023-08-08T09:37:16Z

Description

Resolves #3784

This PR evicts expired TEMPORARY entries. When evicted, entries are deleted from the BucketList (and DB if enabled) and a new kind of meta is emitted to assist downstream systems with garbage collection.

While the eviction scan portion is well tested, I still need to add one more unit test to check the actual meta emitted. This turned out to be non-trivial and is coming soon. I also removed network config's lazy loading. Previously, network config were only loaded from disk if an upgrade occurred. Now that the network config stores an iterator that is updated on each ledger close, this lazy loading no longer makes sense since every ledger is essentially an upgrade. Ideally, we would break network configs and this sort of "validator metadata" (BucketList size snapshots and Eviction iterators) into a new LedgerEntry type, but this would require and XDR change and probably doesn't fit in our timeline.

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

sisuresh · 2023-08-17T22:04:00Z

src/bucket/Bucket.cpp

+            if (isSorobanDataEntry(le.data) && isTemporaryEntry(le.data) &&
+                !isLive(le, ledgerSeq))
+            {
+                if (ltx.maybeEvict(le))


Instead of modifying LedgerTxn to handle eviction, can you just -

Load the entry from ltx.

Verify that the entry in ltx is expired.

Call erase.

I'm trying to reason if LedgerTxn needs to track EntryChangeType. If you just want the meta for entries evicted, I think you should be able to created a nested LedgerTxn, call BucketManager::scanForEviction, and then get the changes from that, which should only contain the meta for the evicted entries.

The issue I was having is that whenever I tried to do some sort of nested LedgerTxn, the meta changes appeared as if they were TX based changes, which is incorrect. I'm not super knowledgeable on the LedgerTxn system though, so if there's a better way to hack it with a nested LedgerTxn I have no objections.

Ok I have it working for the most part with a nested LedgerTxn, but testing is kinda weird. Another thing I noticed is that you assert for LEDGER_ENTRY_REMOVED, but when we scan to delete expired entries, we also emit a LEDGER_ENTRY_STATE (because I call erase in my version). The scan also updates the eviction iterator entry, so those changes will have to be ignored as well.

graydon

Looks really tidy! Just a small handful of nits, nothing huge.

graydon · 2023-08-23T20:02:51Z

src/ledger/InMemoryLedgerTxn.h

@@ -88,6 +88,7 @@ class InMemoryLedgerTxn : public LedgerTxn
    void eraseWithoutLoading(InternalLedgerKey const& key) override;

    LedgerTxnEntry create(InternalLedgerEntry const& entry) override;
+


spurious diff, can remove

graydon · 2023-08-23T20:03:11Z

src/herder/Upgrades.cpp

@@ -3,6 +3,7 @@
 // of this distribution or at http://www.apache.org/licenses/LICENSE-2.0

 #include "herder/Upgrades.h"
+#include "bucket/BucketList.h"


I think spurious (no other edits in this file)

graydon · 2023-08-23T20:06:11Z

src/ledger/LedgerCloseMetaFrame.cpp

+    releaseAssert(mVersion == 2);
+    for (auto const& change : evictionChanges)
+    {
+        if (change.type() != LEDGER_ENTRY_REMOVED)


could you replace this if-with-embedded-if-else with a switch on change.type()? I can't really tell what the intent is of this logic.

graydon · 2023-08-23T20:07:38Z

src/bucket/test/BucketTestUtils.h

-                                           uint32_t ledgerVers) override;
+    void transferLedgerEntriesToBucketList(
+        AbstractLedgerTxn& ltx,
+        std::unique_ptr<LedgerCloseMetaFrame> const& ledgerCloseMeta,


small nit but could we use a std::optional here rather than a unique_ptr-that-might-be-nullptr? Optional sends a stronger "some callers pass nullopt" message than unique_ptr.

Sure, but we already use std::unique for this type of interface in other places like processFeesSeqNums and applyTransactions. Also LedgerCloseMetaFrame stores XDR and is pretty big memory wise, and using std::optional would copy unless we also used std::reference_wrapper, which I'd rather not do.

graydon · 2023-08-23T20:13:15Z

src/bucket/BucketList.h

@@ -441,6 +448,9 @@ class BucketList
    // Returns true if at given `level` dead entries should be kept.
    static bool keepDeadEntries(uint32_t level);

+    // Number of ledgers it takes a bucket to spill/receive an incoming spill
+    static uint32_t bucketChangeRate(uint32_t level, bool isCurr);


nit: rename to bucketUpdateInterval

(or "bucketUpdatePeriod" as you are using in the implementation comment)

graydon · 2023-08-23T20:51:40Z

src/bucket/BucketList.cpp

+{
+#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
+    auto getBucketFromIter = [&levels = mLevels](EvictionIterator const& iter) {
+        auto& level = levels[iter.bucketListLevel];


Minor nit: if you could avoid [...] and use .get(...), at least outside of fast paths, it does a bounds check and gives us an exception on failure rather than just scribbling on memory.

(this is a very general comment about "any access to a std::vector", not specific to this site)

src/bucket/BucketList.cpp

graydon · 2023-08-23T21:23:14Z

src/bucket/BucketList.cpp

+                                           evictionIter.isCurrBucket);
+        if (changeRate * scanSize < b->getSize())
+        {
+            CLOG_WARNING(Bucket,


Assume nobody ever reads warnings -- is this a temporary / harmless thing, or a problem that needs to be addressed by a config setting change, or something else? Should we surface this occurrence in a metric?

This needs to be addressed by a config setting change and is most likely not temporary (essentially if you get this warning, you'll be stuck on the current level until a config upgrade). I can add a metric.

graydon · 2023-08-23T21:41:16Z

src/bucket/Bucket.cpp

+        auto bytesRead = newPos - iter.bucketFileOffset;
+        iter.bucketFileOffset = newPos;
+        bytesScannedForEvictionMeter.inc(bytesRead);
+        if (bytesRead > bytesToScan)


I think this should be >=, right? If you read 100 bytes and your goal was to scan 100 bytes then it's time to stop.

graydon · 2023-08-23T21:47:59Z

src/ledger/NetworkConfig.cpp

+            cfg.stateExpirationSettings().evictionScanSize >=
+                MinimumSorobanNetworkConfig::EVICTION_SCAN_SIZE &&
+            cfg.stateExpirationSettings().startingEvictionScanLevel >=
+                MinimumSorobanNetworkConfig::STARTING_EVICTION_LEVEL;


and < BucketList::kNumLevels

graydon · 2023-08-23T22:19:51Z

src/bucket/BucketList.cpp

+// incoming_spill_frequency(i) = 2^(2i - 1) for i > 0
+// incoming_spill_frequency(0) = 1
+uint32_t
+BucketList::bucketChangeRate(uint32_t level, bool isCurr)


As much as I believe this is likely correct, literally ever time we've done BL arithmetic in the past we've got it slightly wrong; could you add some explicit test vectors on this calculation? just like .. either fish out some specific spills that happened in the history and confirm that they happened this-number-of-ledgers apart, or write a test that just calculates these numbers and then drives the BL forward and checks that the spills happen at the frequency predicted?

graydon · 2023-08-24T17:00:07Z

r+ 5ba6c3a

graydon · 2023-08-24T17:36:38Z

r+ 8250b7b

SirTyson requested review from dmkozh, sisuresh and graydon August 8, 2023 09:37

SirTyson force-pushed the evict-temp-v2 branch 3 times, most recently from ce2ee2e to abd64eb Compare August 15, 2023 17:24

sisuresh reviewed Aug 17, 2023

View reviewed changes

sisuresh mentioned this pull request Aug 18, 2023

Evict temp v2 #3887

Closed

6 tasks

SirTyson force-pushed the evict-temp-v2 branch from 8b7adba to 67851d6 Compare August 22, 2023 22:13

SirTyson and others added 12 commits August 22, 2023 15:14

Added maybeEvict to LedgerTxn

f262764

Basic eviction iterator support

2f7c917

BucketList eviction scan

afb09d0

Added eviction metrics

e28b8b2

Added BucketList eviction tests

7d23fe3

Remove Network Config lazy loading

8344732

Emit eviction meta

4bb5d1f

Fixed BucketList size meta off by one

4757d93

Rebase and fixed test case

c70c46f

Undo LedgerTxn change and use a nested LedgerTxn to get eviction meta

357bd20

Add test

9bdf6ea

Minor test fix and comment

f0fca67

SirTyson force-pushed the evict-temp-v2 branch from 67851d6 to c008ba7 Compare August 22, 2023 23:09

Fixed unitialized memory and loading issue in NetworkConfig tests

1bf2282

SirTyson force-pushed the evict-temp-v2 branch from c008ba7 to 1bf2282 Compare August 23, 2023 03:16

Divide by zero protection

37b3790

graydon requested changes Aug 23, 2023

View reviewed changes

graydon reviewed Aug 23, 2023

View reviewed changes

Cleanup and minor fixes

5ba6c3a

graydon approved these changes Aug 24, 2023

View reviewed changes

Merge branch 'master' into evict-temp-v2

8250b7b

latobarita merged commit c5b96ca into stellar:master Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evict Temporary Entries #3874

Evict Temporary Entries #3874

SirTyson commented Aug 8, 2023 •

edited

Loading

sisuresh Aug 17, 2023 •

edited

Loading

SirTyson Aug 17, 2023

sisuresh Aug 18, 2023

graydon left a comment

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

SirTyson Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

SirTyson Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon Aug 23, 2023

graydon commented Aug 24, 2023

graydon commented Aug 24, 2023

		@@ -88,6 +88,7 @@ class InMemoryLedgerTxn : public LedgerTxn
		void eraseWithoutLoading(InternalLedgerKey const& key) override;

		LedgerTxnEntry create(InternalLedgerEntry const& entry) override;

Evict Temporary Entries #3874

Evict Temporary Entries #3874

Conversation

SirTyson commented Aug 8, 2023 • edited Loading

Description

Checklist

sisuresh Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graydon left a 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

graydon commented Aug 24, 2023

graydon commented Aug 24, 2023

SirTyson commented Aug 8, 2023 •

edited

Loading

sisuresh Aug 17, 2023 •

edited

Loading